On Mon Aug 27 06:36:49 2012, martin.sarfy@sokordia.cz wrote:
Show quoted text> On Sun, Aug 26, 2012 at 10:11 PM, Father Chrysostomos via RT <
> bug-Perl-Critic@rt.cpan.org> wrote:
>
> > <URL:
https://rt.cpan.org/Ticket/Display.html?id=79231 >
> >
> > On Sun Aug 26 14:13:46 2012, martin.sarfy@sokordia.cz wrote:
> > > Hi,
> > >
> > > I am getting complains when I use "return undef".
> > >
> > > I do not think that a simple "return" is always correct. Imagine
> > following
> > > functions:
> > >
> > > sub check { return undef; } # returns boolean
> > > sub list { return () } # returns array
> > > sub find { return wantarray ? () : undef } # returns an object or
> undef
> > >
> > > If you always use "return;", then check() and find() will not work
> as
> > > expected. On the other hand, if you always use "return undef",
> then
> > list()
> > > will be broken.
> > >
> > > The safest method is to always use "return wantarray ? () :
> undef", but
> > > it's too much typing.
> > >
> > > I think it's a bug if Perl::Critic is always requiring "return;",
> because
> > > functions returning "object or undef" are quite common and soon or
> later
> > > you will use such function in a list context (in push or in hash
> > > definition). E.g.:
> > >
> > > sub find { return } # perfectly OK for Perl::Critic
> > > my $x = { object => find(), foo => "bar" };
> > > print "foo => ".$x->{foo}."\n"; # whoops, does NOT print "bar"
> > >
> >
> > Your wantarray method won’t solve that. These are all equivalent:
> >
> > return;
> > return ();
> > return wantarray ? () : undef;
> >
>
> You're right. But in find() above, "return undef" must be used, but
> Perl::Critic does not like it and is suggesting erroneous solution.
>
> (Because of this, I broke my code trying to make it clean...)
>
> M.
>
Any time you make a code change you risk breaking something. Making code
changes recommended by Perl::Critic is no different. I broke a bunch of
my own code implementing Subroutines::RequireArgUnpacking, mostly by
missing 'shift' operations that used the default argument.
Subroutines::ProhibitExplicitReturnUndef is a Perl Best Practices policy
-- that is, it implements a recommendation in that book. But the policy
is not a replacement for the book, and you should see the book for full
details of why Damien Conway thinks it is a good thing to do.
Given compliance to this policy, There's More Than One Way To Do It. My
favorite (because it works also for subroutines you have no control
over, such as localtime()) is to force scalar context. In your find()
example, that would be
sub find { return } # perfectly OK for Perl::Critic
my $x = { object => scalar find(), foo => "bar" };
# ------
print "foo => ".$x->{foo}."\n"; # DOES print "bar"
I also note that, if you follow another Perl::Critic policy and 'use
warnings,' Perl in fact warns you that you have an odd number of values
in a hash in your original code.
That said, you may not agree with either the policy or my recommendation
on how to code with it. That's OK -- Perl::Critic is intended to be a
tool, not a straitjacket. I myself do not use all policies. If you don't
agree with this policy, you have two ways to deal with it:
* If you like the idea in general, but have certain subroutines that you
always want to return a single value from, regardless of context, you
can tell Perl::Critic "I meant to do that", as follows:
sub find {
return undef; ## no critic (ProhibitExplicitReturnUndef)
}
* If you simply believe the policy should never be applied to your code,
put the following line in your .perlcriticrc configuration file.
[-Subroutines::ProhibitExplicitReturnUndef]
Tom
