Skip Menu |
 

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Perl-Critic CPAN distribution.

Report information
The Basics
Id: 67256
Status: open
Priority: 0/
Queue: Perl-Critic

People
Owner: Nobody in particular
Requestors: EDAVIS [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Normal
Broken in: 1.115
Fixed in: (no value)



Subject: ProhibitUnusedCapture false positive
Download (untitled) / with headers
text/plain 448b
Thanks for your recent work on ProhibitUnusedCapture. Unfortunately there is another kind of false positive to report. 'perlcritic -1' on the following code wrongly reports that the captured value is not being used. #!/usr/bin/perl # $Id:$ use warnings; use strict; use Carp qw(croak); our $VERSION = 1; sub foo { my $x = shift; return length $x; } $_ = 'aa'; my @x; foreach (/(a+)/gsxm) { push @x, foo($1); } print $x[0] or croak;
On Tue Apr 05 13:29:47 2011, EDAVIS wrote: Show quoted text
> Thanks for your recent work on ProhibitUnusedCapture. Unfortunately > there is another kind of false positive to report. 'perlcritic -1' on > the following code wrongly reports that the captured value is not being > used. > > #!/usr/bin/perl > # $Id:$ > use warnings; > use strict; > use Carp qw(croak); > our $VERSION = 1; > > sub foo { > my $x = shift; > return length $x; > } > $_ = 'aa'; > my @x; > foreach (/(a+)/gsxm) { > push @x, foo($1); > } > print $x[0] or croak;
It looks to me like this may be technically a bug in ProhibitUnusedCapture. But I wonder why you are using 'foreach' rather than 'while'. The problem with 'foreach' is that except for special cases the list is constructed before the block is ever entered. This means that the $1 in the body of the expression contains the _last_ match on _all_ iterations, which makes the example look very much like a bug. The attached rt67256a.pl illustrates the difference between the behavior of 'while' and 'foreach'.
Subject: rt67256a.pl
Download rt67256a.pl
text/x-perl 193b
#!/usr/local/bin/perl local $_ = 'abc'; my @x; while ( /(\w)/sxmg ) { push @x, $1; } print "while: @x\n"; @x = (); foreach ( /(\w)/sxmg ) { push @x, $1; } print "foreach: @x\n";
Subject: Re: [rt.cpan.org #67256] ProhibitUnusedCapture false positive
Date: Tue, 5 Apr 2011 16:02:44 -0600
To: bug-Perl-Critic [...] rt.cpan.org
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
Download (untitled) / with headers
text/plain 650b
On Apr 5, 2011, at 3:41 PM, Tom Wyant via RT wrote: Show quoted text
> But I wonder why you are using 'foreach' rather than 'while'. The > problem with 'foreach' is that except for special cases the list is > constructed before the block is ever entered. This means that the $1 in > the body of the expression contains the _last_ match on _all_ > iterations, which makes the example look very much like a bug.
I thought map() might be more appropriate here... sub foo {return length shift} local $_ = 'aabaaadaa'; my @x = map { foo($_) } /(a+)/gsxm; Jeffrey Thalhammer Imaginative Software Systems vcard: http://www.imaginative-software.com/contact/jeff.vcf
Download (untitled) / with headers
text/plain 436b
Yes, it's a bug. I should have either used while with $1, or foreach with $_. I wonder whether there should be a policy to warn about this mistake? It could warn about foreach (//g) in general, in which case it would be a standalone policy, or it could only warn on foreach (//g) { ... $1 ... }, in which case it might be included as part of this policy. Anyway, thanks for the correction - I was a bit confused about what //g does.


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

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.