On Sun, Sep 5, 2010 at 15:01, Shlomi Fish <[email protected]> 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,
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}?
> + my (%signo, @signame);
> +
> + my $index = 0;
> +
> + foreach my $name (split(' ', $Config{sig_name})) {
Maybe use "for" since we're touching this'
> + $signo{$name} = $index;
> + $signame[$index] = $name;
> +
> + $index++;
I think using "$index" instead of the obviously temporary and local
"$i" is bad style.
> }
>
> 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.
>
> 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?
> 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.
