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)

History Show all quoted text
  Tue Apr 05 13:29:47 2011 EDAVIS [...] cpan.org - Ticket created
Subject: ProhibitUnusedCapture false positive
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;
  Tue Apr 05 17:41:51 2011 wyant [...] cpan.org - Correspondence added
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
#!/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";
  Tue Apr 05 17:41:52 2011 The RT System itself - Status changed from 'new' to 'open'
  Tue Apr 05 18:02:55 2011 jeff [...] imaginative-software.com - Correspondence added
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>
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
  Wed Apr 06 08:09:53 2011 EDAVIS [...] cpan.org - Correspondence added
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.