Hi, sorry for the late response.
On Sunday 05 September 2010 18:22:02 Ævar Arnfjörð Bjarmason wrote: > On Sun, Sep 5, 2010 at 15:01, Shlomi Fish <shlo...@iglu.org.il> wrote: > > On Sunday 05 September 2010 17:58:54 Shlomi Fish wrote: > >> On Sunday 05 September 2010 08:13:02 Jesse Vincent wrote: > >> > On Sun, Sep 05, 2010 at 08:03:41AM +0300, Shlomi Fish wrote: > >> > > Hi all, > >> > > > >> > > Inspired by a message ot the perl documentation proejct, I started > >> > > working on revamping perlipc.pod here: > >> > > > >> > > http://github.com/shlomif/perl/tree/perlipc-revamp > >> > > > >> > > What I did so far is convert all tabs to spaces (as the indentation > >> > > was very erratic) and started modernising the code > >> > > >> > Shlomi, > >> > > >> > Thanks for starting to look at perlipc. Is there a chance you could > >> > send your patch as a series that splits out the whitespace changes > >> > from the code/prose changes? Heavy whitespace changes tend to make > >> > it much harder to review "contentful" changes in a patch, since there > >> > are so many lines of diff that aren't actually semantically > >> > meaningful. > >> > > >> > Thanks, > >> > Jesse > >> > >> Thanks to the git history, I can. Here is the tabs->spaces patch and the > >> next reply will contain the code/prose changes. I've marked the > >> transition in the repository using the > >> «perlipc_pod_after_changing_tabs_to_spaces» tag. > >> > >> Regards, > >> > >> Shlomi Fish > > > > And here's the code/prose changes patch which works againt the version > > after the tabs->spaces conversion: > > > > Regards, > > > > Shlomi Fish > > > > diff --git a/pod/perlipc.pod b/pod/perlipc.pod > > index 9c556d0..4bc119b 100644 > > --- a/pod/perlipc.pod > > +++ b/pod/perlipc.pod > > @@ -48,11 +48,21 @@ system, or you can retrieve them from the Config > > module. Set up an > > indexed by name to get the number: > > > > use Config; > > - defined $Config{sig_name} || die "No sigs?"; > > - foreach $name (split(' ', $Config{sig_name})) { > > - $signo{$name} = $i; > > - $signame[$i] = $name; > > - $i++; > > + > > + if (!defined $Config{sig_name}) > > + { > > + die "No sigs?"; > > + } > > That's way too verbose. I don't find anything wrong with the original, Well, using || before die is not a good practice. Maybe we can change it to "or". I personally usually use "or die" only after open and friends. > but maybe: > > die "No sigs?" unless defined $Config{sig_name} > > Or even, to depend on recent perls: > > $Config{sig_name} // die "No sigs?"; > > Or maybe skip the whole thing? Are there really modern platforms with > no $Config{sig_name}? Don't know. > > > + my (%signo, @signame); > > + > > + my $index = 0; > > + > > + foreach my $name (split(' ', $Config{sig_name})) { > > Maybe use "for" since we're touching this' > What? The general convention is to use "for" for C-style for loops and "foreach" for iterating over a list. > > + $signo{$name} = $index; > > + $signame[$index] = $name; > > + > > + $index++; > > I think using "$index" instead of the obviously temporary and local > "$i" is bad style. > $index here is local and temporary. Before my patch it was package-scoped. We can argue about whether it should be "$i" or "$index" all day, but I strive for clarity of intentions. > > } > > > > So to check whether signal 17 and SIGALRM were the same, do just this: > > @@ -82,8 +92,9 @@ values are "inherited" by functions called from within > > that block.) > > > > sub precious { > > local $SIG{INT} = 'IGNORE'; > > - &more_functions; > > + more_functions(); > > Good. Using & makes me think "does this actually need to remove a > stack frame" which complexifies the example a lot. > :-). > > } > > + > > sub more_functions { > > # interrupts still ignored, for now... > > } > > @@ -119,7 +130,7 @@ You may be able to determine the cause of failure > > using C<%!>. > > You might also want to employ anonymous functions for simple signal > > handlers: > > > > - $SIG{INT} = sub { die "\nOutta here!\n" }; > > + $SIG{INT} = sub { die "\nOutta here!\n"; }; > > Overly verbose IMO. If we're going to be applying this sort of thing > consistently. But maybe it's easier for people that want to add extra > stuff to the function, dunno. I think one semicolon won't make such a difference. And PBP recommends terminating all statements with a semi-colon even if they are the last ones in a block (and for good reasons). > > > But that will be problematic for the more complicated handlers that need > > to reinstall themselves. Because Perl's signal mechanism is currently > > @@ -169,25 +180,38 @@ example: > > my %children; > > > > $SIG{CHLD} = sub { > > + > > # don't change $! and $? outside handler > > local ($!,$?); > > + > > my $pid = waitpid(-1, WNOHANG); > > + > > return if $pid == -1; > > + > > return unless defined $children{$pid}; > > + > > delete $children{$pid}; > > + > > cleanup_child($pid, $?); > > }; > > Why put \n\n after everything here? Well, I was trying to split the code into paragraphs (again - see PBP), but I may have overdone it. > > > while (1) { > > + > > my $pid = fork(); > > + > > if ($pid == 0) { > > - # ... > > + > > + # I'm the child - do something. > > exit 0; > > - } else { > > - $children{$pid}=1; > > + > > + } > > + else { > > + > > + $children{$pid}=1; > > # ... > > system($command); > > # ... > > + > > } > > } > > > > @@ -202,12 +226,16 @@ using longjmp() or throw() in other languages. > > Here's an example: > > > > eval { > > - local $SIG{ALRM} = sub { die "alarm clock restart" }; > > + > > + local $SIG{ALRM} = sub { die "alarm clock restart"; }; > > + > > alarm 10; > > flock(FH, 2); # blocking write lock > > alarm 0; > > + > > }; > > - if ($@ and $@ !~ /alarm clock restart/) { die } > > + > > + if ($@ and $@ !~ /alarm clock restart/) { die; } > > Since you're touching this anyway adding a message to the die would be > useful. Doesn't it rethrow the exception as it is? Regards, Shlomi Fish -- ----------------------------------------------------------------- Shlomi Fish http://www.shlomifish.org/ "The Human Hacking Field Guide" - http://shlom.in/hhfg <rindolf> She's a hot chick. But she smokes. <go|dfish> She can smoke as long as she's smokin'. Please reply to list if it's a mailing list post - http://shlom.in/reply .
