Bug #40109 for Smart-Comments: for aliasing bug: patch and tests

Hi Damian, I admire your work, so when I found a bug in Smart::Comments, I wanted to impress you and send a fix. The bug can easily be seen with the following code: my @foo_ary = qw<a b c>; for my $foo ( @foo_ary ) { ### Crossing:===| done $foo = 'x'; } say "@foo"; Which prints "x x x" without Smart::Comments, but it prints "a b c" with it. The problem is that the for loop generated by the filter breaks Perls ability to create an alias to each value of the array (at least in 5.10). My patch fixes this, but only if the range of the for loop is a bare array. I started with something more ambitious, but gave up when I realised how hard it would be with ranges like "for ( $money-- )". So you probably need to add something to your doc too about these limitations, even if you don't use the patch. There's two new tests in the patch, "for_aliasing.t", which illuminates the bug, and "for_range_munging.t", which highlights the limitations of the patch. The patch should work from the Smart-Comments-v1.0.3 directory with "patch -Np1 <boldra_patch.diff" cheers Boldra

diff -Naur Smart-Comments-old/lib/Smart/Comments.pm Smart-Comments-v1.0.3/lib/Smart/Comments.pm --- Smart-Comments-old/lib/Smart/Comments.pm 2008-02-22 06:25:30.000000000 +0000 +++ Smart-Comments-v1.0.3/lib/Smart/Comments.pm 2008-10-16 19:14:01.313098300 +0000 @@ -175,13 +175,32 @@ sub _decode_for { my ($for, $range, $mesg) = @_; + #return "my \$not_first__$ID;" + # . "$for (my \@SmartComments__range__$ID = $range) { " + # . "Smart::Comments::_for_progress(" + # . "qq{$mesg}, \$not_first__$ID, \\\@SmartComments__range__$ID);"; # Give the loop a unique ID... $ID++; # Rewrite the loop with a progress bar as its first statement... - return "my \$not_first__$ID;$for (my \@SmartComments__range__$ID = $range) { Smart::Comments::_for_progress(qq{$mesg}, \$not_first__$ID, \\\@SmartComments__range__$ID);"; + + # Let loops with array ranges have access to aliases: + # (you deserve what you get if your array is tied!) + if( $range =~ m/^ \s+ \@ [\w:]+ \s+ $/xms ) { # eg @aaa + return "my \$not_first__$ID;" + . "$for ( $range ) { " + . "my \@SmartComments__range__$ID = $range; " + . "Smart::Comments::_for_progress" + . "(qq{$mesg}, \$not_first__$ID, \\\@SmartComments__range__$ID);"; + } else { + return "my \$not_first__$ID;" + . "$for ( my \@SmartComments__range__$ID = $range ) { " + . "Smart::Comments::_for_progress" + . "(qq{$mesg}, \$not_first__$ID, \\\@SmartComments__range__$ID);"; + } } + # Generate progress-bar code for a Perlish while loop... sub _decode_while { my ($while, $mesg) = @_; diff -Naur Smart-Comments-old/t/for_aliasing.t Smart-Comments-v1.0.3/t/for_aliasing.t --- Smart-Comments-old/t/for_aliasing.t 1970-01-01 00:00:00.000000000 +0000 +++ Smart-Comments-v1.0.3/t/for_aliasing.t 2008-10-16 19:21:15.461012500 +0000 @@ -0,0 +1,25 @@ +use Smart::Comments; +use Test::More tests => 2; + +close *STDERR; +my $STDERR = q{}; +open *STDERR, '>', \$STDERR; + +my @foo_ary = qw<a b c>; + +for my $foo ( @foo_ary ) { ### Crossing:===| done + $foo = 'x'; +} + +is_deeply( \@foo_ary, [ qw<x x x> ], 'loop aliased correctly' ); + + +TODO: { + my @bar_ary = qw<a b c>; + for my $foo ( ( @bar_ary ) ) { ### Crossing:===| done + $foo = 'x'; + } + local $TODO = "testing for a bare array is still very crude"; + is_deeply( \@bar_ary, [ qw<x x x> ], 'loop aliased correctly' ); +} + diff -Naur Smart-Comments-old/t/for_range_munging.t Smart-Comments-v1.0.3/t/for_range_munging.t --- Smart-Comments-old/t/for_range_munging.t 1970-01-01 00:00:00.000000000 +0000 +++ Smart-Comments-v1.0.3/t/for_range_munging.t 2008-10-16 18:43:21.115181700 +0000 @@ -0,0 +1,13 @@ +use Smart::Comments; +use Test::More tests => 1; + +close *STDERR; +my $STDERR = q{}; +open *STDERR, '>', \$STDERR; + + +my $a = 0; +for ( $a++ ) { ### Crossing:===| done + +} +is( $a, 1, 'range not munged' );