From @shlomif

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 .

From @shlomif

From 5fba9e7c503f0a7c80e19f00554358c225c75dc1 Mon Sep 17 00:00:00 2001
From: Shlomi Fish <shlomif@shlomifish.org>
Date: Sat, 21 Feb 2015 17:39:09 +0200
Subject: [PATCH] [perldsc] Modernize the code samples in 2nd part.

This patch modernizes and refactors the code samples, adding my
declarations and some changes.
---
 pod/perldsc.pod | 191 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 108 insertions(+), 83 deletions(-)

diff --git a/pod/perldsc.pod b/pod/perldsc.pod
index e2c4eae..7804231 100644
--- a/pod/perldsc.pod
+++ b/pod/perldsc.pod
@@ -276,8 +276,10 @@ If this is starting to sound scarier than it's worth, relax.  Perl has
 some features to help you avoid its most common pitfalls.  The best
 way to avoid getting confused is to start every program like this:
 
-    #!/usr/bin/perl -w
+    #!/usr/bin/perl
+
     use strict;
+    use warnings;
 
 This way, you'll be forced to declare all your variables with my() and
 also disallow accidental "symbolic dereferencing".  Therefore if you'd done
@@ -336,7 +338,7 @@ X<array of arrays> X<AoA>
 
 =head2 Declaration of an ARRAY OF ARRAYS
 
- @AoA = (
+ my @AoA = (
         [ "fred", "barney" ],
         [ "george", "jane", "elroy" ],
         [ "homer", "marge", "bart" ],
@@ -350,13 +352,14 @@ X<array of arrays> X<AoA>
  }
 
  # calling a function
- for $i ( 1 .. 10 ) {
+ # Note that $AoA[0] will not be initialised.
+ for my $i ( 1 .. 10 ) {
      $AoA[$i] = [ somefunc($i) ];
  }
 
  # using temp vars
- for $i ( 1 .. 10 ) {
-     @tmp = somefunc($i);
+ for my $i ( 1 .. 10 ) {
+     my @tmp = somefunc($i);
      $AoA[$i] = [ @tmp ];
  }
 
@@ -372,19 +375,20 @@ X<array of arrays> X<AoA>
  $AoA[1][1] =~ s/(\w)/\u$1/;
 
  # print the whole thing with refs
- for $aref ( @AoA ) {
+ for my $aref ( @AoA ) {
      print "\t [ @$aref ],\n";
  }
 
  # print the whole thing with indices
- for $i ( 0 .. $#AoA ) {
+ for my $i ( 0 .. $#AoA ) {
      print "\t [ @{$AoA[$i]} ],\n";
  }
 
  # 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";
+ for my $i ( 0 .. $#AoA ) {
+     my $row = $AoA[$i];
+     for my $j ( 0 .. $#{ $row } ) {
+         print "elt $i $j is $row->[$j]\n";
      }
  }
 
@@ -393,7 +397,7 @@ X<hash of arrays> X<HoA>
 
 =head2 Declaration of a HASH OF ARRAYS
 
- %HoA = (
+ my %HoA = (
         flintstones        => [ "fred", "barney" ],
         jetsons            => [ "george", "jane", "elroy" ],
         simpsons           => [ "homer", "marge", "bart" ],
@@ -410,19 +414,19 @@ X<hash of arrays> X<HoA>
 
  # reading from file; more temps
  # flintstones: fred barney wilma dino
- while ( $line = <> ) {
-     ($who, $rest) = split /:\s*/, $line, 2;
-     @fields = split ' ', $rest;
+ while ( my $line = <> ) {
+     my ($who, $rest) = split /:\s*/, $line, 2;
+     my @fields = split ' ', $rest;
      $HoA{$who} = [ @fields ];
  }
 
  # calling a function that returns a list
- for $group ( "simpsons", "jetsons", "flintstones" ) {
+ for my $group ( "simpsons", "jetsons", "flintstones" ) {
      $HoA{$group} = [ get_family($group) ];
  }
 
  # likewise, but using temps
- for $group ( "simpsons", "jetsons", "flintstones" ) {
+ for my $group ( "simpsons", "jetsons", "flintstones" ) {
      @members = get_family($group);
      $HoA{$group} = [ @members ];
  }
@@ -439,26 +443,27 @@ X<hash of arrays> X<HoA>
  $HoA{simpsons}[1] =~ s/(\w)/\u$1/;
 
  # print the whole thing
- foreach $family ( keys %HoA ) {
+ foreach my $family ( keys %HoA ) {
      print "$family: @{ $HoA{$family} }\n"
  }
 
  # print the whole thing with indices
- foreach $family ( keys %HoA ) {
+ foreach my $family ( keys %HoA ) {
      print "family: ";
-     foreach $i ( 0 .. $#{ $HoA{$family} } ) {
-         print " $i = $HoA{$family}[$i]";
+     my $family_members = $HoA{$family};
+     foreach $i ( 0 .. $#{ $family_members } ) {
+         print " $i = $family_members->[$i]";
      }
      print "\n";
  }
 
  # print the whole thing sorted by number of members
- foreach $family ( sort { @{$HoA{$b}} <=> @{$HoA{$a}} } keys %HoA ) {
+ foreach my $family ( sort { @{$HoA{$b}} <=> @{$HoA{$a}} } keys %HoA ) {
      print "$family: @{ $HoA{$family} }\n"
  }
 
  # print the whole thing sorted by number of members and name
- foreach $family ( sort {
+ foreach my $family ( sort {
                             @{$HoA{$b}} <=> @{$HoA{$a}}
                                         ||
                                     $a cmp $b
@@ -472,7 +477,7 @@ X<array of hashes> X<AoH>
 
 =head2 Declaration of an ARRAY OF HASHES
 
- @AoH = (
+ my @AoH = (
         {
             Lead     => "fred",
             Friend   => "barney",
@@ -494,9 +499,9 @@ X<array of hashes> X<AoH>
  # reading from file
  # format: LEAD=fred FRIEND=barney
  while ( <> ) {
-     $rec = {};
+     my $rec = {};
      for $field ( split ) {
-         ($key, $value) = split /=/, $field;
+         my ($key, $value) = split /=/, $field;
          $rec->{$key} = $value;
      }
      push @AoH, $rec;
@@ -512,7 +517,7 @@ X<array of hashes> X<AoH>
 
  # calling a function  that returns a key/value pair list, like
  # "lead","fred","daughter","pebbles"
- while ( %fields = getnextpairset() ) {
+ while ( my %fields = getnextpairset() ) {
      push @AoH, { %fields };
  }
 
@@ -534,27 +539,29 @@ X<array of hashes> X<AoH>
  $AoH[1]{lead} =~ s/(\w)/\u$1/;
 
  # print the whole thing with refs
- for $href ( @AoH ) {
+ for my $href ( @AoH ) {
      print "{ ";
-     for $role ( keys %$href ) {
+     for my $role ( keys %$href ) {
          print "$role=$href->{$role} ";
      }
      print "}\n";
  }
 
  # print the whole thing with indices
- for $i ( 0 .. $#AoH ) {
+ for my $i ( 0 .. $#AoH ) {
      print "$i is { ";
-     for $role ( keys %{ $AoH[$i] } ) {
-         print "$role=$AoH[$i]{$role} ";
+     my $href = $AoH[$i];
+     for $role ( keys %{ $href } ) {
+         print "$role=$href->{$role} ";
      }
      print "}\n";
  }
 
  # print the whole thing one at a time
- for $i ( 0 .. $#AoH ) {
-     for $role ( keys %{ $AoH[$i] } ) {
-         print "elt $i $role is $AoH[$i]{$role}\n";
+ for my $i ( 0 .. $#AoH ) {
+     my $href = $AoH[$i];
+     for $role ( keys %{ $href } ) {
+         print "elt $i $role is $href->{$role}\n";
      }
  }
 
@@ -563,7 +570,7 @@ X<hash of hashes> X<HoH>
 
 =head2 Declaration of a HASH OF HASHES
 
- %HoH = (
+ my %HoH = (
         flintstones => {
                 lead      => "fred",
                 pal       => "barney",
@@ -584,48 +591,56 @@ X<hash of hashes> X<HoH>
 
  # reading from file
  # flintstones: lead=fred pal=barney wife=wilma pet=dino
+ LINES:
  while ( <> ) {
-     next unless s/^(.*?):\s*//;
-     $who = $1;
-     for $field ( split ) {
-         ($key, $value) = split /=/, $field;
+     next LINES unless s/^(.*?):\s*//;
+     my $who = $1;
+     for my $field ( split ) {
+         my ($key, $value) = split /=/, $field;
          $HoH{$who}{$key} = $value;
      }
 
 
  # reading from file; more temps
+ LINES:
  while ( <> ) {
-     next unless s/^(.*?):\s*//;
-     $who = $1;
-     $rec = {};
+     next LINES unless s/^(.*?):\s*//;
+     my $who = $1;
+     my $rec = {};
      $HoH{$who} = $rec;
-     for $field ( split ) {
-         ($key, $value) = split /=/, $field;
+     for my $field ( split ) {
+         my ($key, $value) = split /=/, $field;
          $rec->{$key} = $value;
      }
  }
 
  # calling a function  that returns a key,value hash
- for $group ( "simpsons", "jetsons", "flintstones" ) {
+ for my $group ( "simpsons", "jetsons", "flintstones" ) {
      $HoH{$group} = { get_family($group) };
  }
 
  # likewise, but using temps
- for $group ( "simpsons", "jetsons", "flintstones" ) {
-     %members = get_family($group);
+ for my $group ( "simpsons", "jetsons", "flintstones" ) {
+     my %members = get_family($group);
      $HoH{$group} = { %members };
  }
 
  # append new members to an existing family
- %new_folks = (
+ my %new_folks = (
      wife => "wilma",
      pet  => "dino",
  );
 
- for $what (keys %new_folks) {
+ for my $what (keys %new_folks) {
      $HoH{flintstones}{$what} = $new_folks{$what};
  }
 
+ # Alternatively
+ {
+     my $family = $HoH{flintstones};
+     %{ $family } = (%{ $family }, %new_folks);
+ }
+
 =head2 Access and Printing of a HASH OF HASHES
 
  # one element
@@ -635,43 +650,52 @@ X<hash of hashes> X<HoH>
  $HoH{simpsons}{lead} =~ s/(\w)/\u$1/;
 
  # print the whole thing
- foreach $family ( keys %HoH ) {
+ foreach my $family ( keys %HoH ) {
      print "$family: { ";
-     for $role ( keys %{ $HoH{$family} } ) {
-         print "$role=$HoH{$family}{$role} ";
+     my $family_record = $HoH{$family};
+     for my $role ( keys %{ $family_record } ) {
+         print "$role=$family_record->{$role} ";
      }
      print "}\n";
  }
 
  # print the whole thing  somewhat sorted
- foreach $family ( sort keys %HoH ) {
+ foreach my $family ( sort keys %HoH ) {
      print "$family: { ";
-     for $role ( sort keys %{ $HoH{$family} } ) {
-         print "$role=$HoH{$family}{$role} ";
+     my $family_record = $HoH{$family};
+     for my $role ( sort keys %{ $family_record } ) {
+         print "$role=$family_record->{$role} ";
      }
      print "}\n";
  }
 
 
  # print the whole thing sorted by number of members
- foreach $family ( sort { keys %{$HoH{$b}} <=> keys %{$HoH{$a}} } keys %HoH ) {
+ foreach my $family ( sort { keys %{$HoH{$b}} <=> keys %{$HoH{$a}} } keys %HoH ) {
      print "$family: { ";
-     for $role ( sort keys %{ $HoH{$family} } ) {
-         print "$role=$HoH{$family}{$role} ";
+     my $family_record = $HoH{$family};
+     for my $role ( sort keys %{ $family_record } ) {
+         print "$role=$family_record->{$role} ";
      }
      print "}\n";
  }
 
  # establish a sort order (rank) for each role
- $i = 0;
- for ( qw(lead wife son daughter pal pet) ) { $rank{$_} = ++$i }
+ my %rank;
+ {
+    my $i = 0;
+    for my $key ( qw(lead wife son daughter pal pet) ) {
+        $rank{$_} = ++$i;
+    }
+ }
 
  # now print the whole thing sorted by number of members
- foreach $family ( sort { keys %{ $HoH{$b} } <=> keys %{ $HoH{$a} } } keys %HoH ) {
+ foreach my $family ( sort { keys %{ $HoH{$b} } <=> keys %{ $HoH{$a} } } keys %HoH ) {
      print "$family: { ";
+     my $family_record = $HoH{$family};
      # and print these according to rank order
-     for $role ( sort { $rank{$a} <=> $rank{$b} }  keys %{ $HoH{$family} } ) {
-         print "$role=$HoH{$family}{$role} ";
+     for my $role ( sort { $rank{$a} <=> $rank{$b} }  keys %{ $family_record } ) {
+         print "$role=$family_record->{$role} ";
      }
      print "}\n";
  }
@@ -685,7 +709,7 @@ X<record> X<structure> X<struct>
 Here's a sample showing how to create and use a record whose fields are of
 many different sorts:
 
-     $rec = {
+     my $rec = {
          TEXT      => $string,
          SEQUENCE  => [ @old_values ],
          LOOKUP    => { %some_table },
@@ -697,24 +721,23 @@ many different sorts:
      print $rec->{TEXT};
 
      print $rec->{SEQUENCE}[0];
-     $last = pop @ { $rec->{SEQUENCE} };
+     my $last = pop @ { $rec->{SEQUENCE} };
 
      print $rec->{LOOKUP}{"key"};
-     ($first_k, $first_v) = each %{ $rec->{LOOKUP} };
+     my ($first_k, $first_v) = each %{ $rec->{LOOKUP} };
 
-     $answer = $rec->{THATCODE}->($arg);
-     $answer = $rec->{THISCODE}->($arg1, $arg2);
+     my $answer = $rec->{THATCODE}->($arg);
+     my $second_answer = $rec->{THISCODE}->($arg1, $arg2);
 
      # careful of extra block braces on fh ref
      print { $rec->{HANDLE} } "a string\n";
 
-     use FileHandle;
      $rec->{HANDLE}->autoflush(1);
      $rec->{HANDLE}->print(" a string\n");
 
 =head2 Declaration of a HASH OF COMPLEX RECORDS
 
-     %TV = (
+     my %TV = (
         flintstones => {
             series   => "flintstones",
             nights   => [ qw(monday thursday friday) ],
@@ -755,14 +778,14 @@ many different sorts:
      # sometimes it's easiest to do that
 
      # here's a piece by piece build up
-     $rec = {};
+     my $rec = {};
      $rec->{series} = "flintstones";
      $rec->{nights} = [ find_days() ];
 
-     @members = ();
+     my @members;
      # assume this file in field=value syntax
      while (<>) {
-         %fields = split /[\s=]+/;
+         my %fields = split /[\s=]+/;
          push @members, { %fields };
      }
      $rec->{members} = [ @members ];
@@ -778,10 +801,10 @@ many different sorts:
      # to an array of the kids' records without having duplicate
      # records and thus update problems.
      ###########################################################
-     foreach $family (keys %TV) {
-         $rec = $TV{$family}; # temp pointer
-         @kids = ();
-         for $person ( @{ $rec->{members} } ) {
+     foreach my $family (keys %TV) {
+         my $rec = $TV{$family}; # temp pointer
+         my @kids;
+         for my $person ( @{ $rec->{members} } ) {
              if ($person->{role} =~ /kid|son|daughter/) {
                  push @kids, $person;
              }
@@ -803,16 +826,18 @@ many different sorts:
      # both point to the same underlying anonymous hash table
 
      # print the whole thing
-     foreach $family ( keys %TV ) {
+     foreach my $family ( keys %TV ) {
+         my $family_record = $TV{$family};
          print "the $family";
-         print " is on during @{ $TV{$family}{nights} }\n";
+         print " is on during @{ $family_record->{nights} }\n";
          print "its members are:\n";
-         for $who ( @{ $TV{$family}{members} } ) {
+         for my $who ( @{ $family_record->{members} } ) {
              print " $who->{name} ($who->{role}), age $who->{age}\n";
          }
-         print "it turns out that $TV{$family}{lead} has ";
-         print scalar ( @{ $TV{$family}{kids} } ), " kids named ";
-         print join (", ", map { $_->{name} } @{ $TV{$family}{kids} } );
+         print "it turns out that $family_record->{lead} has ";
+         my $kids = $family_record->{kids};
+         print scalar ( @{ $kids } ), " kids named ";
+         print join (", ", map { $_->{name} } @{ $kids } );
          print "\n";
      }
 
-- 
2.3.0

From @jkeenan

On Sat Feb 21 07​:49​:48 2015, shlomif@​shlomifish.org wrote​:

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)

The RT System itself - Status changed from 'new' to 'open'

From @ap

* 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.

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.

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/>

From @shlomif

Hi Aristotle and James,

sorry for the late reply.

On Sat Feb 21 14​:34​:24 2015, aristotle wrote​:

* 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.

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/^\(\.\*?\)&#8203;:\\s\*//;
\+     next LINES unless s/^\(\.\*?\)&#8203;:\\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.

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?

From @rjbs

* 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.

\+ LINES​:
  while \( \<> \) \{
\-     next unless s/^\(\.\*?\)&#8203;:\\s\*//;
\+     next LINES unless s/^\(\.\*?\)&#8203;:\\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.

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

From @shlomif

On Wed Feb 25 19​:17​:45 2015, perl.p5p@​rjbs.manxome.org wrote​:

* 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.

+ 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.

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

From @rjbs

* 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.)

--
rjbs

From @jkeenan

On Tue, 03 Mar 2015 01​:08​:30 GMT, perl.p5p@​rjbs.manxome.org wrote​:

* 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)

@jkeenan - Status changed from 'open' to 'rejected'