Re: [PATCH] Start revamping perlipc.pod

Hi,

sorry for the late response.

On Sunday 05 September 2010 18:22:02 =C3=86var Arnfj=C3=B6r=C3=B0 Bjarmason=
 wrote:
> 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,
> >> > >=20
> >> > > Inspired by a message ot the perl documentation proejct, I started
> >> > > working on revamping perlipc.pod here:
> >> > >=20
> >> > > http://github.com/shlomif/perl/tree/perlipc-revamp
> >> > >=20
> >> > > What I did so far is convert all tabs to spaces (as the indentation
> >> > > was very erratic) and started modernising the code
> >> >=20
> >> > Shlomi,
> >> >=20
> >> > 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.
> >> >=20
> >> > Thanks,
> >> > Jesse
> >>=20
> >> Thanks to the git history, I can. Here is the tabs->spaces patch and t=
he
> >> next reply will contain the code/prose changes. I've marked the
> >> transition in the repository using the
> >> =C2=ABperlipc_pod_after_changing_tabs_to_spaces=C2=BB tag.
> >>=20
> >> Regards,
> >>=20
> >>       Shlomi Fish
> >=20
> > And here's the code/prose changes patch which works againt the version
> > after the tabs->spaces conversion:
> >=20
> > Regards,
> >=20
> >        Shlomi Fish
> >=20
> > 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:
> >=20
> >     use Config;
> > -    defined $Config{sig_name} || die "No sigs?";
> > -    foreach $name (split(' ', $Config{sig_name})) {
> > -        $signo{$name} =3D $i;
> > -        $signame[$i] =3D $name;
> > -        $i++;
> > +
> > +    if (!defined $Config{sig_name})
> > +    {
> > +        die "No sigs?";
> > +    }
>=20
> 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=
=20
"or". I personally usually use "or die" only after open and friends.

> but maybe:
>=20
>     die "No sigs?" unless defined $Config{sig_name}
>=20
> Or even, to depend on recent perls:
>=20
>     $Config{sig_name} // die "No sigs?";
>=20
> Or maybe skip the whole thing? Are there really modern platforms with
> no $Config{sig_name}?

Don't know.

>=20
> > +    my (%signo, @signame);
> > +
> > +    my $index =3D 0;
> > +
> > +    foreach my $name (split(' ', $Config{sig_name})) {
>=20
> Maybe use "for" since we're touching this'
>=20

What?

The general convention is to use "for" for C-style for loops and "foreach" =
for=20
iterating over a list.

> > +        $signo{$name} =3D $index;
> > +        $signame[$index] =3D $name;
> > +
> > +        $index++;
>=20
> I think using "$index" instead of the obviously temporary and local
> "$i" is bad style.
>=20

$index here is local and temporary. Before my patch it was package-scoped. =
We=20
can argue about whether it should be "$i" or "$index" all day, but I strive=
=20
for clarity of intentions.


> >     }
> >=20
> >  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.)
> >=20
> >     sub precious {
> >         local $SIG{INT} =3D 'IGNORE';
> > -        &more_functions;
> > +        more_functions();
>=20
> Good. Using & makes me think "does this actually need to remove a
> stack frame" which complexifies the example a lot.
>

:-).
=20
> >     }
> > +
> >     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:
> >=20
> > -    $SIG{INT} =3D sub { die "\nOutta here!\n" };
> > +    $SIG{INT} =3D sub { die "\nOutta here!\n"; };
>=20
> 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=20
terminating all statements with a semi-colon even if they are the last ones=
 in=20
a block (and for good reasons).

>=20
> >  But that will be problematic for the more complicated handlers that ne=
ed
> >  to reinstall themselves.  Because Perl's signal mechanism is currently
> > @@ -169,25 +180,38 @@ example:
> >     my %children;
> >=20
> >     $SIG{CHLD} =3D sub {
> > +
> >         # don't change $! and $? outside handler
> >         local ($!,$?);
> > +
> >         my $pid =3D waitpid(-1, WNOHANG);
> > +
> >         return if $pid =3D=3D -1;
> > +
> >         return unless defined $children{$pid};
> > +
> >         delete $children{$pid};
> > +
> >         cleanup_child($pid, $?);
> >     };
>=20
> Why put \n\n after everything here?

Well, I was trying to split the code into paragraphs (again - see PBP), but=
 I=20
may have overdone it.=20

>=20
> >     while (1) {
> > +
> >         my $pid =3D fork();
> > +
> >         if ($pid =3D=3D 0) {
> > -            # ...
> > +
> > +            # I'm the child - do something.
> >             exit 0;
> > -        } else {
> > -        $children{$pid}=3D1;
> > +
> > +        }
> > +        else {
> > +
> > +            $children{$pid}=3D1;
> >             # ...
> >             system($command);
> >             # ...
> > +
> >        }
> >     }
> >=20
> > @@ -202,12 +226,16 @@ using longjmp() or throw() in other languages.
> >  Here's an example:
> >=20
> >     eval {
> > -        local $SIG{ALRM} =3D sub { die "alarm clock restart" };
> > +
> > +        local $SIG{ALRM} =3D 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; }
>=20
> 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

=2D-=20
=2D----------------------------------------------------------------
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 .