Re: [PATCH] Start revamping perlipc.pod

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 se=
nd
>> > your patch as a series that splits out the whitespace changes from the
>> > code/prose changes? =C2=A0Heavy whitespace changes tend to make it muc=
h
>> > 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 transiti=
on
>> in the repository using the =C2=ABperlipc_pod_after_changing_tabs_to_spa=
ces=C2=BB
>> tag.
>>
>> Regards,
>>
>> =C2=A0 =C2=A0 =C2=A0 Shlomi Fish
>>
>
> And here's the code/prose changes patch which works againt the version af=
ter
> the tabs->spaces conversion:
>
> Regards,
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0Shlomi 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 modu=
le.
> Set up an
> =C2=A0indexed by name to get the number:
>
> =C2=A0 =C2=A0 use Config;
> - =C2=A0 =C2=A0defined $Config{sig_name} || die "No sigs?";
> - =C2=A0 =C2=A0foreach $name (split(' ', $Config{sig_name})) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0$signo{$name} =3D $i;
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0$signame[$i] =3D $name;
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0$i++;
> +
> + =C2=A0 =C2=A0if (!defined $Config{sig_name})
> + =C2=A0 =C2=A0{
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0die "No sigs?";
> + =C2=A0 =C2=A0}

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}?

> + =C2=A0 =C2=A0my (%signo, @signame);
> +
> + =C2=A0 =C2=A0my $index =3D 0;
> +
> + =C2=A0 =C2=A0foreach my $name (split(' ', $Config{sig_name})) {

Maybe use "for" since we're touching this'

> + =C2=A0 =C2=A0 =C2=A0 =C2=A0$signo{$name} =3D $index;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0$signame[$index] =3D $name;
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0$index++;

I think using "$index" instead of the obviously temporary and local
"$i" is bad style.

> =C2=A0 =C2=A0 }
>
> =C2=A0So to check whether signal 17 and SIGALRM were the same, do just th=
is:
> @@ -82,8 +92,9 @@ values are "inherited" by functions called from within =
that
> block.)
>
> =C2=A0 =C2=A0 sub precious {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 local $SIG{INT} =3D 'IGNORE';
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0&more_functions;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0more_functions();

Good. Using & makes me think "does this actually need to remove a
stack frame" which complexifies the example a lot.

> =C2=A0 =C2=A0 }
> +
> =C2=A0 =C2=A0 sub more_functions {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 # interrupts still ignored, for now...
> =C2=A0 =C2=A0 }
> @@ -119,7 +130,7 @@ You may be able to determine the cause of failure usi=
ng
> C<%!>.
> =C2=A0You might also want to employ anonymous functions for simple signal
> =C2=A0handlers:
>
> - =C2=A0 =C2=A0$SIG{INT} =3D sub { die "\nOutta here!\n" };
> + =C2=A0 =C2=A0$SIG{INT} =3D 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.

>
> =C2=A0But that will be problematic for the more complicated handlers that=
 need
> =C2=A0to reinstall themselves. =C2=A0Because Perl's signal mechanism is c=
urrently
> @@ -169,25 +180,38 @@ example:
> =C2=A0 =C2=A0 my %children;
>
> =C2=A0 =C2=A0 $SIG{CHLD} =3D sub {
> +
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 # don't change $! and $? outside handler
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 local ($!,$?);
> +
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 my $pid =3D waitpid(-1, WNOHANG);
> +
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 return if $pid =3D=3D -1;
> +
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 return unless defined $children{$pid};
> +
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 delete $children{$pid};
> +
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 cleanup_child($pid, $?);
> =C2=A0 =C2=A0 };

Why put \n\n after everything here?

> =C2=A0 =C2=A0 while (1) {
> +
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 my $pid =3D fork();
> +
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ($pid =3D=3D 0) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0# ...
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0# I'm the child - do something=
.
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 exit 0;
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0} else {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0$children{$pid}=3D1;
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0else {
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0$children{$pid}=3D1;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # ...
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 system($command);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # ...
> +
> =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> =C2=A0 =C2=A0 }
>
> @@ -202,12 +226,16 @@ using longjmp() or throw() in other languages.
> =C2=A0Here's an example:
>
> =C2=A0 =C2=A0 eval {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0local $SIG{ALRM} =3D sub { die "alarm clock =
restart" };
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0local $SIG{ALRM} =3D sub { die "alarm clock =
restart"; };
> +
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 alarm 10;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 flock(FH, 2); =C2=A0 # blocking write lock
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 alarm 0;
> +
> =C2=A0 =C2=A0 };
> - =C2=A0 =C2=A0if ($@ and $@ !~ /alarm clock restart/) { die }
> +
> + =C2=A0 =C2=A0if ($@ and $@ !~ /alarm clock restart/) { die; }

Since you're touching this anyway adding a message to the die would be
useful.