[perl #41186] [BUG]: tools/build/ops2pm.pl: Variable used in both package scope and loop iteration
James Keenan
# Please include the string: [perl #41186]
# in the subject line of all future correspondence about this issue.
# <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=41186 >
Particle has asked me to consider testing and refactoring
('phalanxing', for short) tools/build/ops2pm.pl and tools/build/
ops2c.pl in the same way that I did tools/build/pmc2c.pl.
In my first look at ops2pm.pl, I noticed a possible bug. $file
starts off as a package-scoped lexical variable. It is assigned the
first element remaining on @ARGV at the point of assignment.
115 my $file = shift @ARGV;
The script dies if $file does not exist or turns out not to be a
suitable argument for Parrot::OpsFile::new().
$file next turns up as the iterator variable in a 'for' loop over the
remaining elements in @ARGV:
126 for $file (@ARGV) {
127 if ( $seen{$file} ) {
...
150 }
I would normally have expected to see the iterator variable
differently named and lexically scoped to the 'for' loop:
for my $f (@ARGV) {
... in order to preserve $file for use in the script after the end of
the 'for' loop. At this point there's no big problem, because the
intention of the script is to process a series of files named on the
command line, the first of which is processed a bit differently from
all subsequent ones. But we have to realize that our package-scoped
$file is being assigned to -- albeit less than explicitly -- on each
iteration of the 'for' loop.
The problem arises later on when $file is called inside two HERE docs
being printed to newly created files.
225 my $preamble = <<END_C;
226 #! perl -w
227 #
228 # !!!!!!! DO NOT EDIT THIS FILE !!!!!!!
229 #
230 # This file is generated automatically from '$file'.
...
244 END_C
and
268 print $OUT <<END_C;
269 /* ex: set ro:
270 * !!!!!!! DO NOT EDIT THIS FILE !!!!!!!
271 *
272 * This file is generated automatically from '$file'
273 * by $0.
...
282 END_C
As I read this, the value of $file which will be used in these two
HERE docs is the *last* value assigned to $file when it was used as
the iterator of the 'for' loop -- which should be the *last* command-
line argument.
Is this what we're actually getting when make call ops2pm.pl?
Is this what is intended? If not, shouldn't we either (a)
distinguish between the package-scoped $file and the 'for' loop
iterator $file; and/or (b) make a provision for explicit assignment
to $file just before the HERE docs?
kid51
James E Keenan
> # New Ticket Created by James Keenan
> # Please include the string: [perl #41186]
> # in the subject line of all future correspondence about this issue.
> # <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=41186 >
>
>
> Particle has asked me to consider testing and refactoring
> ('phalanxing', for short) tools/build/ops2pm.pl and tools/build/
> ops2c.pl in the same way that I did tools/build/pmc2c.pl.
>
> In my first look at ops2pm.pl, I noticed a possible bug. $file
> starts off as a package-scoped lexical variable. It is assigned the
> first element remaining on @ARGV at the point of assignment.
>
>
> $file next turns up as the iterator variable in a 'for' loop over the
> remaining elements in @ARGV:
>
>
> HERE docs is the *last* value assigned to $file when it was used as
> the iterator of the 'for' loop -- which should be the *last* command-
> line argument.
>
I spoke too soon, i.e., I spoke before constructing a test.
local @ARGV = qw( alpha beta gamma delta );
my $file = shift(@ARGV);
print "$file\n";
for $file (@ARGV) {
print "Seeing $file\n";
}
print "At last $file\n";
... which prints:
alpha
Seeing beta
Seeing gamma
Seeing delta
At last alpha
But I *still* don't think it's a good practice to use $file both as a
package-scoped lexical and as a loop iterator. It confused me; it'll
confuse somebody else.
kid51
James E Keenan
> # New Ticket Created by James Keenan
> # Please include the string: [perl #41186]
> # in the subject line of all future correspondence about this issue.
> # <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=41186 >
>
>
> Particle has asked me to consider testing and refactoring
> ('phalanxing', for short) tools/build/ops2pm.pl and tools/build/
> ops2c.pl in the same way that I did tools/build/pmc2c.pl.
>
> In my first look at ops2pm.pl, I noticed a possible bug. $file
> starts off as a package-scoped lexical variable. It is assigned the
> first element remaining on @ARGV at the point of assignment.
>
>
> $file next turns up as the iterator variable in a 'for' loop over the
> remaining elements in @ARGV:
>
>
> HERE docs is the *last* value assigned to $file when it was used as
> the iterator of the 'for' loop -- which should be the *last* command-
> line argument.
>
I spoke too soon, i.e., I spoke before constructing a test.
James Keenan via RT
February.