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: 56624
Status: open
Priority: 0/
Queue: Perl-Critic

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

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



Subject: Double-sigil dereference $% (possibly false positive)
Download (untitled) / with headers
text/plain 247b
Double-sigil dereference at line 7, column 7. See page 228 of PBP. 1 #!/usr/bin/perl 2 use strict; 3 4 my %hash; 5 my $ref = \%hash; 6 7 print %$href; The use of "%$" is quite common, likewise for the "@$ref"
Subject: Re: [rt.cpan.org #56624] Double-sigil dereference $% (possibly false positive)
Date: Thu, 15 Apr 2010 08:05:21 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Download (untitled) / with headers
text/plain 304b
On 4/15/10 5:47 AM, Jari Aalto via RT wrote: Show quoted text
> The use of "%$" is quite common, likewise for the "@$ref"
Just because something is common doesn't make it right. Just because eval { blah }; if ($@) { blah blah } is common doesn't mean that it doesn't have a serious bug in it.
Download (untitled) / with headers
text/plain 635b
On Thu Apr 15 09:05:37 2010, clonezone wrote: Show quoted text
> On 4/15/10 5:47 AM, Jari Aalto via RT wrote:
> > The use of "%$" is quite common, likewise for the "@$ref"
> > Just because something is common doesn't make it right. > > Just because > > eval { blah }; > if ($@) { > blah blah > } > > is common doesn't mean that it doesn't have a serious bug in it.
That's different. The bug in question is about: <sigil><sigil><full variable name> where <full variable name> := simple expressions as in [\w_]+ or something. That should be handled more carefully and not label everything <sigil><sigil> as suspect.
Subject: Re: [rt.cpan.org #56624] Double-sigil dereference $% (possibly false positive)
Date: Wed, 21 Apr 2010 20:42:19 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Download (untitled) / with headers
text/plain 612b
On 4/15/10 8:14 AM, Jari Aalto via RT wrote: Show quoted text
> That should be handled more carefully and not label everything > <sigil><sigil> as suspect.
If you pay attention to what Conway is talking about, he is not discussing bugs. He's talking about communicating intent. A significant portion of Perl developers don't understand the precedence of the expression "$$foo->bar()": does it mean "${$foo->bar()}" or does it mean "${$foo}->bar()"? You and I may know the answer, but Conway's point is that most people do not, so he wants you to always separate your sigils. If you don't agree with the policy, disable it.
Download (untitled) / with headers
text/plain 1.4k
On Wed Apr 21 21:42:33 2010, clonezone wrote: Show quoted text
> On 4/15/10 8:14 AM, Jari Aalto via RT wrote:
> > That should be handled more carefully and not label everything > > <sigil><sigil> as suspect.
> > If you pay attention to what Conway is talking about, he is not > discussing bugs. He's talking about communicating intent. A > significant portion of Perl developers don't understand the > precedence of the expression "$$foo->bar()": does it mean "${$foo-
> >bar()}" or does it mean "${$foo}->bar()"? You and I may know the
> answer, but Conway's point is that most people do not, so he wants > you to always separate your sigils.
Sure, a message for complex expressions is ok, but the bug report and my last comment was addressed to: <full variable name> := simple expression That doesn't include "$$foo->" Labeling everything suspect is not a good idea. If someone wants to to be fanatic, he may consider including cases like @$var (and the like) to be a suspect. But in practice it would be hard to find a Perl coder that would find code like above to be "ambiguous" or prone to "misinterpretation". Any added punctuation to that will make it less clear in spirit of "less is more" (according to the XP methodology in programming). TO RECAP --------- Fine tune the warning: - allow simple cases and do not emit warnings on those: <sigil><sigil><straight simple variable> Better: offer configuration to bypass simple cases and continue to emit warnings from more complex cases.
Subject: Re: [rt.cpan.org #56624] Double-sigil dereference $% (possibly false positive)
Date: Sat, 24 Apr 2010 12:54:00 -0700
To: bug-Perl-Critic [...] rt.cpan.org
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
Download (untitled) / with headers
text/plain 1.1k
On Apr 24, 2010, at 12:03 PM, Jari Aalto via RT wrote: Show quoted text
> TO RECAP > --------- > > Fine tune the warning: > > - allow simple cases and do not emit warnings on those: > <sigil><sigil><straight simple variable> > > Better: offer configuration to bypass simple cases and continue to emit > warnings from more complex cases.
I see your point, and I am strongly considering adding this flexibility. But aside from detecting bugs and making code easier to read, perlcritic also strives to enforce consistency. And sometimes, consistency is more valuable than flexibility. For new code, it is pretty easy to get into the habit of using curly-braces to separate the sigils in both complex and simple expressions. But when critiquing legacy code, it is tempting to try and bend Perl::Critic to fit your existing habits. Sometimes this makes perfect sense, and sometimes not. I think the key thing to consider is whether one truly objects to a Policy (such as it is), or if he/she just don't have the time & will to make all their code compliant. Your mileage may vary. Jeffrey Thalhammer Imaginative Software Systems vcard: http://www.imaginative-software.com/contact/jeff.vcf


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.