Useless use of const in void context in lib/unicore/mktables

[richardleach]

Description
Poking around for #23448 showed a possible bug in line 16093 of lib/unicore/mktables:

my $sub_filename = join('/', $directory_ref->[1, -1], $file);

which will behave as:

my $sub_filename = join('/', $directory_ref->[-1], $file);

because the 1 is in scalar context and gets optimised away:

1    list LISTOP(0x561f9b37ace8) ===> 2 [aelem 0x561f9b37ac70]
     PARENT ===> 2 [aelem 0x561f9b37ac70]
     FLAGS = (SCALAR,KIDS,SLABBED)
     |   
3    +--pushmark OP(0x561f9b37ad60) ===> 4 [const 0x561f9b37ad28]
     |   FLAGS = (VOID,SLABBED,MORESIB)
     |   
5    +--null (ex-const) OP(0x561f9b37ad98) ===> 4 [const 0x561f9b37ad28]
     |   FLAGS = (VOID,SLABBED,MORESIB)
     |   
4    +--const SVOP(0x561f9b37ad28) ===> 1 [list 0x561f9b37ace8]
         FLAGS = (SCALAR,SLABBED,FOLDED)
         SV = IV(-1)

(There's no warning emitted because of the special treatment of constants 0 and 1.)

Expected behavior
I'm not familiar with the operation of this script, so don't know if this line is behaving
as intended (and could have the 1, removed) or is subtly buggy.

Perl configuration
blead

[bram-perl]

Relevant code:

        # Associate it with its file internally.  Don't include the
        # $matches_directory first component
        $table->set_file_path(@$directory_ref, $file);

        # No swash means don't do the rest of this.
        next if $table->isa('Map_Table') && $table->fate != $ORDINARY;

        my $sub_filename = join('/', $directory_ref->[1, -1], $file);

Looking at the history it started as: (commit 99870f4):

        # Associate it with its file internally.  Don't include the
        # $matches_directory first component
        $table->set_file_path(@$directory_ref, $file);
        my $sub_filename = join('/', $directory_ref->[1, -1], $file);

and the "no swash" block was added in between at a later point (commit c15fda2)

I believe the Don't include the $matches_directory first component comment applies to the $sub_filename line but the addition of the "swash"-block makes this less clear.

Based on the comment I suppose the code is bogus to begin with and should've been something like (untested): join('/', @$directory_ref[1 .. $#directory_ref], $file);

The real question of course is does any of this matter? What is $sub_filename being used for...

[jkeenan]

Description Poking around for #23448 showed a possible bug in line 16093 of lib/unicore/mktables:

my $sub_filename = join('/', $directory_ref->[1, -1], $file);

which will behave as:

my $sub_filename = join('/', $directory_ref->[-1], $file);

because the 1 is in scalar context and gets optimised away:

@khwilliamson, the area in question was written by you way back when:

99870f4d255 (Karl Williamson          2009-11-21 23:45:28 -0700 16093)         my $sub_filename = join('/', $directory_ref->[1, -1], $file);

Can you take a look? Thanks.

[jkeenan]

@richardleach, I'm not persuaded that there's a real problem here.

Before I explain why, let me first acknowledge that I have never looked at lib/unicore/mktables before this week. I could not explain its purpose or operation to you. All I have done is to (1) add print STDERR statements in the region of your concern; (2) run make and redirect its output to a file for examination; and (3) try alternate ways of assigning to $sub_filename and see if this makes any difference in the files created by this part of make.

You can see my debugging branches here:

https://github.com/jkeenan/perl5/tree/2-in-1

https://github.com/jkeenan/perl5/tree/alt-2-in-1

If you compare blead with 2-in-1, you get:

$ git diff -w blead..2-in-1 -- lib/unicore/mktables | cat
diff --git a/lib/unicore/mktables b/lib/unicore/mktables
index 0e0d786c4b..945f847038 100644
--- a/lib/unicore/mktables
+++ b/lib/unicore/mktables
@@ -16015,6 +16015,9 @@ sub register_file_for_name($table, $directory_ref, $file) {
     # $directory_ref    # Array of the directory path for the file
     # $file             # The file name in the final directory.
 
+    print STDERR "AAA: ref \$table: ", ref($table), "\n";
+    print STDERR "BBB: array-directory_ref: <", join(' ' => @{$directory_ref}), ">\n";
+    print STDERR "CCC: file: $file\n";
     trace "table=$table, file=$file, directory=@$directory_ref, fate=", $table->fate if main::DEBUG && $to_trace;
 
     if ($table->isa('Property')) {
@@ -16070,16 +16073,14 @@ sub register_file_for_name($table, $directory_ref, $file) {
         $directory_ref =\@directories;
     }
 
-    # Join all the file path components together, using slashes.
-    my $full_filename = join('/', @$directory_ref, $file);
-
     # All go in the same subdirectory of unicore, or the special
     # pseudo-directory '#'
     if ($directory_ref->[0] !~ / ^ $matches_directory | \# $ /x) {
         Carp::my_carp("Unexpected directory in "
                 .  join('/', @{$directory_ref}, $file));
     }
-
+    print STDERR "KKK: directory-ref\n";
+    for my $el (@{$directory_ref}) { print STDERR "  <$el>\n" }
     # For this table and all its equivalents ...
     foreach my $table ($table, $table->equivalents) {
 
@@ -16090,7 +16091,12 @@ sub register_file_for_name($table, $directory_ref, $file) {
         # No swash means don't do the rest of this.
         next if $table->isa('Map_Table') && $table->fate != $ORDINARY;
 
+        print STDERR "LLL: ", $directory_ref->[1, -1], "\n"; 
+        print STDERR "MMM: ", $file, "\n"; 
         my $sub_filename = join('/', $directory_ref->[1, -1], $file);
+        print STDERR "NNN: $sub_filename\n"; 
+        my $alt_sub_filename = join('/', $directory_ref->[1, $#$directory_ref], $file);
+        print STDERR "OOO: $alt_sub_filename\n"; 
 
         my $property = $table->property;
         my $property_name = ($property == $perl)

If you compare 2-in-1 with alt-2-in-1, you get:

$ git diff -w 2-in-1..alt-2-in-1 -- lib/unicore/mktables | cat
diff --git a/lib/unicore/mktables b/lib/unicore/mktables
index 945f847038..45812521e2 100644
--- a/lib/unicore/mktables
+++ b/lib/unicore/mktables
@@ -16093,10 +16093,11 @@ sub register_file_for_name($table, $directory_ref, $file) {
 
         print STDERR "LLL: ", $directory_ref->[1, -1], "\n"; 
         print STDERR "MMM: ", $file, "\n"; 
-        my $sub_filename = join('/', $directory_ref->[1, -1], $file);
-        print STDERR "NNN: $sub_filename\n"; 
+#        my $sub_filename = join('/', $directory_ref->[1, -1], $file);
+#        print STDERR "NNN: $sub_filename\n"; 
         my $alt_sub_filename = join('/', $directory_ref->[1, $#$directory_ref], $file);
         print STDERR "OOO: $alt_sub_filename\n"; 
+        my $sub_filename = $alt_sub_filename;
 
         my $property = $table->property;
         my $property_name = ($property == $perl)

I built each branch, then copied the *.pl and *.pm files from lib/unicore to a per-branch tempdir. I then diffed the tempdirs. The only difference was the SHA for lib/unicore/mktables in lib/unicore/uni_keywords.pl.

$ diff -w -r fe78a34bc3/ 507a04690a/
diff -w -r fe78a34bc3/uni_keywords.pl 507a04690a/uni_keywords.pl
1406c1406
< # 1ecfdddd59776cff68bdc9a99913f5488e88450d2fddc7b8a5af105cb21394e6 lib/unicore/mktables
---
> # 84185f95dea2051186622dc3fe8e7c29cedb6a4c7029d7a50ae84cabc702c671 lib/unicore/mktables

I saw no warnings and got no exceptions here. So, setting aside my own beginner status with the code in this file, I don't yet see evidence of a bug.

[bram-perl]

Before I explain why, let me first acknowledge that I have never looked at lib/unicore/mktables before this week. I could not explain its purpose or operation to you. All I have done is to (1) add print STDERR statements in the region of your concern; (2) run make and redirect its output to a file for examination; and (3) try alternate ways of assigning to $sub_filename and see if this makes any difference in the files created by this part of make.

You can see my debugging branches here:

Your debugging is wrong and not changing anything.

Basically you went from:

        my $sub_filename = join('/', $directory_ref->[1, -1], $file);

to:

        my $alt_sub_filename = join('/', $directory_ref->[1, $#$directory_ref], $file);

Which changes nothing. In both cases the "1" is still a useless constant in void context.

Let's make it a bit more obvious:

#!/usr/bin/perl

use strict;
use warnings;

my $directory_ref = [ "a" .. "z" ];
my $file = "bar.txt";

my $sub_filename = join('/', $directory_ref->[1, -1], $file);
my $alt_sub_filename = join('/', $directory_ref->[1, $#$directory_ref], $file);

print "sub_filename=$sub_filename\n";
print "alt_sub_filename=$alt_sub_filename\n";

Output:

sub_filename=z/bar.txt
alt_sub_filename=z/bar.txt

What @richardleach point is/was:

• the 1 is a useless constant but it doesn't get warned on because 1 is special cased
• it might be a subtle bug in the code that went unnoticed (due the lack of warning).

Let's change the code a bit so it does warn: (changing the 1 to 3):

my $sub_filename = join('/', $directory_ref->[3, -1], $file);
my $alt_sub_filename = join('/', $directory_ref->[3, $#$directory_ref], $file);

And running:

Useless use of a constant (3) in void context at gh-23520.pl line 9.
Useless use of a constant (3) in void context at gh-23520.pl line 10.
sub_filename=z/bar.txt
alt_sub_filename=z/bar.txt

What my point is/was is that the code should probably have been something like:

my $sub_filename = join('/', @$directory_ref[1 .. $#$directory_ref], $file);

i.e. a dereferenced array slice.

(Or for extra clarity: my @x = @$directory_ref; shift @x; join('/', @x, $file) --> i.e. take all of $directory_ref except the first element (with index 0))

Doing that:

#!/usr/bin/perl

use strict;
use warnings;

my $directory_ref = [ "a" .. "z" ];
my $file = "bar.txt";

my $sub_filename = join('/', $directory_ref->[3, -1], $file);
my $alt_sub_filename = join('/', @$directory_ref[3 .. $#$directory_ref], $file);

print "sub_filename=$sub_filename\n";
print "alt_sub_filename=$alt_sub_filename\n";

And running:

Useless use of a constant (3) in void context at gh-23520.pl line 9.
sub_filename=z/bar.txt
alt_sub_filename=d/e/f/g/h/i/j/k/l/m/n/o/p/q/r/s/t/u/v/w/x/y/z/bar.txt

The question is not if @richardleach change makes a difference (it does not), the question is if the code contains a subtle bug.

So to know that you can update your debugging code to use something like:

my $alt_sub_filename = join('/', @$directory_ref[1 .. $#$directory_ref], $file);

and see if that changes any of the output files.

It's very important to note that @$x[4 .. 8] is not the same as $x->[4 .. 8]:

• join("", @$x[4, 7]) is equal to: @y = @$x; join("", @y[4, 7]) which is equal to: $x->[4] . $x->[7] (or join("", $x->@[4, 7]) with postfix def)
• join("", $x->[4, 7]) is equal to: @y = @$x; join("", $y[4, 7] which is equal to: $x->[7] (+ a bunch of warnings)
• join("", @$x[4 .. 7]) is equal to: @y = @$x; join("", @y[4 .. 7]) which is equal to: $x->[4] . $x->[5] . $x->[6] . $x->[7] (or join("", $x->@[4 .. 7]))
• $. = 1; join("", $x->[1 .. 7]) is equal to: @y = @$x; join("", $y[1 .. 7]) which is equal to $x->[1]. (In this case the .. is not the range operator but the flip-flop operator)