Skip Menu |
Report information
Id: 123898
Status: rejected
Priority: 0/
Queue: perl5

Owner: jkeenan <jkeenan [at] cpan.org>
Requestors: shlomif [at] shlomifish.org
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: low
Type:
Perl Version: (no value)
Fixed In: (no value)



Date: Sat, 21 Feb 2015 17:49:30 +0200
From: Shlomi Fish <shlomif [...] shlomifish.org>
Subject: [PATCH] Modernize the second part of perldsc.pod.
To: perlbug [...] perl.org
Download (untitled) / with headers
text/plain 529b
This patch modernizes the second part of perldsc.pod. It adds declarations using my, replaces "-w" with "use warnings", extracts repeating expressions into variables, and some other changes. Regards, Shlomi Fish -- ----------------------------------------------------------------- Shlomi Fish http://www.shlomifish.org/ My Aphorisms - http://www.shlomifish.org/humour.html E‐mail, web feeds, and doing something productive — choose two. Please reply to list if it's a mailing list post - http://shlom.in/reply .

Message body is not shown because sender requested not to inline it.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.7k
On Sat Feb 21 07:49:48 2015, shlomif@shlomifish.org wrote: Show quoted text
> This patch modernizes the second part of perldsc.pod. It adds declarations > using my, replaces "-w" with "use warnings", extracts repeating expressions > into variables, and some other changes. >
Shlomi, In some of the cases where you "extract repeating expressions into variables," I think that that extraction is not needed and just adds one more line to the code example. For example, under the section "Access and Printing of an ARRAY OF ARRAYS", we currently have this code example (blead, starting at line 387): ##### # print the whole thing one at a time for $i ( 0 .. $#AoA ) { for $j ( 0 .. $#{ $AoA[$i] } ) { print "elt $i $j is $AoA[$i][$j]\n"; } } ##### After your revision, this section would read: ##### # print the whole thing one at a time for my $i ( 0 .. $#AoA ) { my $row = $AoA[$i]; for my $j ( 0 .. $#{ $row } ) { print "elt $i $j is $row->[$j]\n"; } } ##### The assignment to $row *does* mean that the lines in the inner loop are easier to read. For training someone in Perl, that's perfectly fine. But $row is, IMO, an example of what MJD long ago described as a "synthetic variable" ripe for refactoring away. AFAICT, it does not reduce the number of array element lookups we have to do subsequently. I'm currently attuned to this because I've been refactoring one of my older CPAN distributions with the objective of speeding up its performance without changing the interface. I've found that eliminating unnecessary assignments to synthetic variables is an easy way to get a measurable (though usually modest) improvement in function performance. The other changes in this patch are, IMO, quite satisfactory. Thank you very much. -- James E Keenan (jkeenan@cpan.org)
From: Aristotle Pagaltzis <pagaltzis [...] gmx.de>
Date: Sat, 21 Feb 2015 23:33:50 +0100
To: perl5-porters [...] perl.org
Subject: Re: [perl #123898] [PATCH] Modernize the second part of perldsc.pod.
Download (untitled) / with headers
text/plain 3.6k
* James E Keenan via RT <perlbug-followup@perl.org> [2015-02-21 21:00]: Show quoted text
> ##### > # print the whole thing one at a time > for $i ( 0 .. $#AoA ) { > for $j ( 0 .. $#{ $AoA[$i] } ) { > print "elt $i $j is $AoA[$i][$j]\n"; > } > } > ##### > > After your revision, this section would read: > ##### > # print the whole thing one at a time > for my $i ( 0 .. $#AoA ) { > my $row = $AoA[$i]; > for my $j ( 0 .. $#{ $row } ) { > print "elt $i $j is $row->[$j]\n"; > } > } > ##### > > The assignment to $row *does* mean that the lines in the inner loop > are easier to read. For training someone in Perl, that's perfectly > fine. But $row is, IMO, an example of what MJD long ago described as > a "synthetic variable" ripe for refactoring away. AFAICT, it does not > reduce the number of array element lookups we have to do subsequently.
Uh, every iteration of the inner loop is spared the lookup of $AoA[$i]. However, while I *would* perform this refactoring on real code, I find it a change for the worse in this instance in that it conceals the correspondence between the structure of the code and the data structure. For the same reason I dislike the corresponding change in the HoA and AoH blocks titled “print the whole thing with indices” near line 440 and line 550, respectively. OTOH, with the correspondence illustrated, I don’t have a problem with e.g. the “print the whole thing one at a time” block from the respective same section pulling out the common subexpression. Show quoted text
> The other changes in this patch are, IMO, quite satisfactory.
Most are unobjectionable. There is one style change I strenuously object to, however. E.g.: # reading from file # flintstones: lead=fred pal=barney wife=wilma pet=dino + LINES: while ( <> ) { - next unless s/^(.*?):\s*//; + next LINES unless s/^(.*?):\s*//; (Repeat some two dozen times.) I cannot find a single case where this helps comprehension. It just adds noise. Every single time. The added `my`s, OTOH, are all benign. But in two cases their addition is insufficiently deliberate. First is the “using temp vars” block near line 360, which is changed from for $i ( 1 .. 10 ) { @tmp = somefunc($i); $AoA[$i] = [ @tmp ]; } to for my $i ( 1 .. 10 ) { my @tmp = somefunc($i); $AoA[$i] = [ @tmp ]; } The added `my`s are fine, but consider the “after” version. If you have just declared @tmp inside the loop body to hold the return values of a function call, why then flatten it in an anonymous array constructor, which makes a copy of its contents? Just take a reference to it: for my $i ( 1 .. 10 ) { my @tmp = somefunc($i); $AoA[$i] = \@tmp; } Note that changing [ @tmp ] to \@tmp in the “before” version would be incorrect – it would break the code: because @tmp was not scoped to the loop, taking a reference to it any number of times would just repeatedly yield a reference to the same array – whose contents are overwritten on each iteration. Therefore the “before” version *needs* to make a copy of the array before adding a ref to @AoA. Once you scope @tmp to the loop body, though, the copying becomes a waste of cycles and memory. There is a likewise shortfall of editing further down, where while (<>) { %fields = split /[\s=]+/; push @members, { %fields }; } becomes while (<>) { my %fields = split /[\s=]+/; push @members, { %fields }; } when it should become while (<>) { my %fields = split /[\s=]+/; push @members, \%fields; } That’s all I can spot at a cursory glance. Regards, -- Aristotle Pagaltzis // <http://plasmasturm.org/>
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 3.6k
Hi Aristotle and James, sorry for the late reply. On Sat Feb 21 14:34:24 2015, aristotle wrote: Show quoted text
> * James E Keenan via RT <perlbug-followup@perl.org> [2015-02-21 21:00]:
> > ##### > > # print the whole thing one at a time > > for $i ( 0 .. $#AoA ) { > > for $j ( 0 .. $#{ $AoA[$i] } ) { > > print "elt $i $j is $AoA[$i][$j]\n"; > > } > > } > > ##### > > > > After your revision, this section would read: > > ##### > > # print the whole thing one at a time > > for my $i ( 0 .. $#AoA ) { > > my $row = $AoA[$i]; > > for my $j ( 0 .. $#{ $row } ) { > > print "elt $i $j is $row->[$j]\n"; > > } > > } > > ##### > > > > The assignment to $row *does* mean that the lines in the inner loop > > are easier to read. For training someone in Perl, that's perfectly > > fine. But $row is, IMO, an example of what MJD long ago described as > > a "synthetic variable" ripe for refactoring away. AFAICT, it does not > > reduce the number of array element lookups we have to do subsequently.
> > Uh, every iteration of the inner loop is spared the lookup of $AoA[$i]. > > However, while I *would* perform this refactoring on real code, I find > it a change for the worse in this instance in that it conceals the > correspondence between the structure of the code and the data structure. >
I see - well, I'll wait for a consensus or veto here. Show quoted text
> For the same reason I dislike the corresponding change in the HoA and > AoH blocks titled “print the whole thing with indices” near line 440 and > line 550, respectively. > > OTOH, with the correspondence illustrated, I don’t have a problem with > e.g. the “print the whole thing one at a time” block from the respective > same section pulling out the common subexpression. >
> > The other changes in this patch are, IMO, quite satisfactory.
> > Most are unobjectionable. There is one style change I strenuously object > to, however. E.g.: > > # reading from file > # flintstones: lead=fred pal=barney wife=wilma pet=dino > + LINES: > while ( <> ) { > - next unless s/^(.*?):\s*//; > + next LINES unless s/^(.*?):\s*//; > > (Repeat some two dozen times.) > > I cannot find a single case where this helps comprehension. It just adds > noise. Every single time.
I added the LINES label per the best practice of http://perl-begin.org/tutorials/bad-elements/#flow-stmts-without-labels (which also appears in the book Perl Best Practices). If there's a consensus or a veto that it shouldn't be added, then I'll remove it in the redone patch. Show quoted text
> > The added `my`s, OTOH, are all benign. But in two cases their addition > is insufficiently deliberate. First is the “using temp vars” block near > line 360, which is changed from > > for $i ( 1 .. 10 ) { > @tmp = somefunc($i); > $AoA[$i] = [ @tmp ]; > } > > to > > for my $i ( 1 .. 10 ) { > my @tmp = somefunc($i); > $AoA[$i] = [ @tmp ]; > } > > The added `my`s are fine, but consider the “after” version. If you have > just declared @tmp inside the loop body to hold the return values of > a function call, why then flatten it in an anonymous array constructor, > which makes a copy of its contents? Just take a reference to it: > > for my $i ( 1 .. 10 ) { > my @tmp = somefunc($i); > $AoA[$i] = \@tmp; > } >
Code like that (= declaring a lexical aggregate variable and storing a reference to it in a container variable) is exactly what the text that I removed from the previous patchset to perldsc.pod warned against doing (and I thought such a warning was no longer necessary in this day and age). So what is the verdict regarding doing it?
Subject: Re: [perl #123898] [PATCH] Modernize the second part of perldsc.pod.
To: Shlomi Fish via RT <perlbug-followup [...] perl.org>
Date: Wed, 25 Feb 2015 22:17:20 -0500
CC: ;, perl5-porters [...] perl.org
From: Ricardo Signes <perl.p5p [...] rjbs.manxome.org>
Download (untitled) / with headers
text/plain 2.5k
* Shlomi Fish via RT <perlbug-followup@perl.org> [2015-02-25T04:27:55] Show quoted text
> On Sat Feb 21 14:34:24 2015, aristotle wrote:
> > Uh, every iteration of the inner loop is spared the lookup of $AoA[$i]. > > > > However, while I *would* perform this refactoring on real code, I find > > it a change for the worse in this instance in that it conceals the > > correspondence between the structure of the code and the data structure.
> > I see - well, I'll wait for a consensus or veto here.
I am with Aristotle. While I'd make the change in product code, I think the documentation is better as written for the purposes of demonstration. Show quoted text
> > + LINES: > > while ( <> ) { > > - next unless s/^(.*?):\s*//; > > + next LINES unless s/^(.*?):\s*//; > > > > (Repeat some two dozen times.) > > > > I cannot find a single case where this helps comprehension. It just adds > > noise. Every single time.
> > I added the LINES label per the best practice of > http://perl-begin.org/tutorials/bad-elements/#flow-stmts-without-labels > (which also appears in the book Perl Best Practices). If there's a consensus > or a veto that it shouldn't be added, then I'll remove it in the redone > patch.
Here, I'm not with Aristotle except in the practical outcome. I nearly always use this pattern, as I find it practical when doing later refactoring (but not because of skimmability). On the other hand, I would not change the documentation. I think it's clearer without the label. There is less to take in without it. Show quoted text
> > The added `my`s are fine, but consider the “after” version. If you have > > just declared @tmp inside the loop body to hold the return values of > > a function call, why then flatten it in an anonymous array constructor, > > which makes a copy of its contents? Just take a reference to it: > > > > for my $i ( 1 .. 10 ) { > > my @tmp = somefunc($i); > > $AoA[$i] = \@tmp; > > }
> > Code like that (= declaring a lexical aggregate variable and storing a > reference to it in a container variable) is exactly what the text that I > removed from the previous patchset to perldsc.pod warned against doing (and I > thought such a warning was no longer necessary in this day and age). So what > is the verdict regarding doing it?
I had a flip through the history on that file and I couldn't find what you're referring to — but to be fair, I gave up pretty fast. If we know that @tmp is quite local to the loop, which we do, I'd do what Aristotle recommends. Is your previous change relevant to that? If so, could you point it out to me? -- rjbs
Download signature.asc
application/pgp-signature 473b

Message body not shown because it is not plain text.

RT-Send-CC: perl5-porters [...] perl.org
On Wed Feb 25 19:17:45 2015, perl.p5p@rjbs.manxome.org wrote: Show quoted text
> * Shlomi Fish via RT <perlbug-followup@perl.org> [2015-02-25T04:27:55]
> > On Sat Feb 21 14:34:24 2015, aristotle wrote:
> > > Uh, every iteration of the inner loop is spared the lookup of > > > $AoA[$i]. > > > > > > However, while I *would* perform this refactoring on real code, I > > > find > > > it a change for the worse in this instance in that it conceals the > > > correspondence between the structure of the code and the data > > > structure.
> > > > I see - well, I'll wait for a consensus or veto here.
> > I am with Aristotle. While I'd make the change in product code, I > think the > documentation is better as written for the purposes of demonstration. >
Very well, then, I'll change it back. Show quoted text
> > > + LINES: > > > while ( <> ) { > > > - next unless s/^(.*?):\s*//; > > > + next LINES unless s/^(.*?):\s*//; > > > > > > (Repeat some two dozen times.) > > > > > > I cannot find a single case where this helps comprehension. It just > > > adds > > > noise. Every single time.
> > > > I added the LINES label per the best practice of > > http://perl-begin.org/tutorials/bad-elements/#flow-stmts-without- > > labels > > (which also appears in the book Perl Best Practices). If there's a > > consensus > > or a veto that it shouldn't be added, then I'll remove it in the > > redone > > patch.
> > Here, I'm not with Aristotle except in the practical outcome. I > nearly always > use this pattern, as I find it practical when doing later refactoring > (but not > because of skimmability). On the other hand, I would not change the > documentation. I think it's clearer without the label. There is less > to take > in without it.
OK. Show quoted text
>
> > > The added `my`s are fine, but consider the “after” version. If you > > > have > > > just declared @tmp inside the loop body to hold the return values > > > of > > > a function call, why then flatten it in an anonymous array > > > constructor, > > > which makes a copy of its contents? Just take a reference to it: > > > > > > for my $i ( 1 .. 10 ) { > > > my @tmp = somefunc($i); > > > $AoA[$i] = \@tmp; > > > }
> > > > Code like that (= declaring a lexical aggregate variable and storing > > a > > reference to it in a container variable) is exactly what the text > > that I > > removed from the previous patchset to perldsc.pod warned against > > doing (and I > > thought such a warning was no longer necessary in this day and age). > > So what > > is the verdict regarding doing it?
> > I had a flip through the history on that file and I couldn't find what > you're > referring to — but to be fair, I gave up pretty fast. If we know that > @tmp is > quite local to the loop, which we do, I'd do what Aristotle > recommends. > > Is your previous change relevant to that? If so, could you point it > out to me?
Sorry, that was an awkward phrasing on my part. I meant that I wanted to remove a paragraph which warned about doing exactly that and that it was considered still relevant, and was vetoed against. I'm referring to: [QUOTE] That's because my() is more of a run-time statement than it is a compile-time declaration I<per se>. This means that the my() variable is remade afresh each time through the loop. So even though it I<looks> as though you stored the same variable reference each time, you actually did -not! This is a subtle distinction that can produce more efficient code at -the risk of misleading all but the most experienced of programmers. So I -usually advise against teaching it to beginners. In fact, except for -passing arguments to functions, I seldom like to see the gimme-a-reference -operator (backslash) used much at all in code. Instead, I advise -beginners that they (and most of the rest of us) should try to use the -much more easily understood constructors C<[]> and C<{}> instead of -relying upon lexical (or dynamic) scoping and hidden reference-counting to -do the right thing behind the scenes. +not! [/QUOTE] And now I was told to convert the code to do exactly that. So what should we do? Regards, -- Shlomi Fish
Date: Mon, 2 Mar 2015 20:08:00 -0500
From: Ricardo Signes <perl.p5p [...] rjbs.manxome.org>
Subject: Re: [perl #123898] [PATCH] Modernize the second part of perldsc.pod.
To: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.5k
* Shlomi Fish via RT <perlbug-followup@perl.org> [2015-02-26T02:38:21] Show quoted text
> Sorry, that was an awkward phrasing on my part. I meant that I wanted to > remove a paragraph which warned about doing exactly that and that it was > considered still relevant, and was vetoed against. I'm referring to:
Thanks, this helped. Sorry for my delayed reply, I had this message open in a tab waiting for when I could read all the bits and pieces relevant to it at once. I agree: there are a couple things in conflict here. Here is my thinking: Tom's summary is: $AoA[$i] = [ @array ]; # usually best $AoA[$i] = \@array; # perilous; just how my() was that array? @{ $AoA[$i] } = @array; # way too tricky for most programmers The question about whether to use \@array is about how certain you are that the array is truly lexical. On one hand, you might say that in this example, it is painfully obvious that @array is lexical. On the other, you can say that it's best to stop worrying about accounting for obviousness, and just pick the safe option. Tom's advice is the latter, at least for beginners. I think perldsc is largely a document for beginners. Let's stick with [ @tmp ] here, for the sake of consistency. It isn't what I'd really write, but not everything in the document has to be. Possibly your previous patch can be revisited to separate things out. I might be more amenible to something cautioning one to be careful when taking references, but just removing the the objection to using backslash is too much. (There were other changes in there that I thought were unjustified and unrelated, too.) -- rjbs
Download signature.asc
application/pgp-signature 473b

Message body not shown because it is not plain text.

RT-Send-CC: perl5-porters [...] perl.org
On Tue, 03 Mar 2015 01:08:30 GMT, perl.p5p@rjbs.manxome.org wrote: Show quoted text
> * Shlomi Fish via RT <perlbug-followup@perl.org> [2015-02-26T02:38:21]
> > Sorry, that was an awkward phrasing on my part. I meant that I wanted > > to > > remove a paragraph which warned about doing exactly that and that it > > was > > considered still relevant, and was vetoed against. I'm referring to:
> > Thanks, this helped. Sorry for my delayed reply, I had this message > open in a > tab waiting for when I could read all the bits and pieces relevant to > it at > once. > > I agree: there are a couple things in conflict here. Here is my > thinking: > > Tom's summary is: > > $AoA[$i] = [ @array ]; # usually best > $AoA[$i] = \@array; # perilous; just how my() was that array? > @{ $AoA[$i] } = @array; # way too tricky for most programmers > > The question about whether to use \@array is about how certain you are > that the > array is truly lexical. On one hand, you might say that in this > example, it is > painfully obvious that @array is lexical. On the other, you can say > that it's > best to stop worrying about accounting for obviousness, and just pick > the safe > option. Tom's advice is the latter, at least for beginners. > > I think perldsc is largely a document for beginners. Let's stick with > [ @tmp ] > here, for the sake of consistency. It isn't what I'd really write, > but not > everything in the document has to be. > > Possibly your previous patch can be revisited to separate things out. > I might > be more amenible to something cautioning one to be careful when taking > references, but just removing the the objection to using backslash is > too much. > (There were other changes in there that I thought were unjustified and > unrelated, too.)
There wasn't sufficient consensus in the discussion in this RT to warrant going forward with any patch at this time. So I'm marking this ticket Rejected but we'll be open to a patch in a new ticket that takes into account the discussion herein. Thank you very much. -- James E Keenan (jkeenan@cpan.org)


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org