Re: Perl::Critic Tests

On Tue, Apr 25, 2017 at 5:12 AM, R. Ransbottom <..hidden..> wrote:

Don't do this, please.

That is Perl novice thinking which shows a poor understanding of Perl's
definition--and use of--context.

The bare return is a basic building block of some standard and
simpler Perl idioms.

My interpretation of it all is that Matt claims (and I fully understand his position) that these simpler idioms are a source for bugs because the resulting program may not do what the unsuspecting programmer thinks it does, especially in the context of returning empty lists.

If we promote bad standards of coding we will discourage the better
coders from joining in on our projects.

True, but elegant code which has a high(er) risk of being insecure can't be construed as "better code", can it? The programmers we'd like to attract on the project would be those who put secure coding over everything else. In my book that means that they write clear and concise code.

The second characteristic we'd look for in people who want to join is that they (want to learn to) love to write tests for the code they deliver. (And so on, and so on...)

On Sat, Apr 22, 2017 at 07:37:25AM +0100, Nick Prater wrote:

> Matt Trout raised an issue that this policy leads to surprising

> sub get_value {
> return;
> }
> my %hash = (
> row => get_value(),
> id => 1
> );
> print Dumper \%hash;
> ----
>
> which returns:
> $VAR1 = {
> '1' => undef,
> 'row' => 'id'
> };

Matt forgot to 'use warnings;'. Ouch! Had me going for a minute.
That example has nothing to do with the bare return statement.

actually, it does have to do with a bare return statement, because Perl decides itself to make the return value `()`. I'm with Nick here: the code did run to get the warning, meaning that the damage has already been done. Warnings don't prevent code from being unsecure. They might help to detect cases after the fact thought.

Witness:

%h = ( "george" );

gives "george" => undef and warns

> I prefer an explicit "return undef;" as I think it's clearer, more
> obvious about the intention and less magical. Eric suggests we problably
> want to switch from "no return at all" to "return undef".

The problem with this _expression_ of intent is that it is buried at
the bottom of a function that someone wants to use not to read.

The point here was that for clarity of code, it's best to have a function either return an array/list or a scalar. While returning "one or the other based on context" results in some elegant code, I'm with Matt that it does not solve the problem of casting values from scalars to arrays and back in a program of moderate size. But if it doesn't, then maybe it's better to have clear intentions all the way.

As for "burried at the bottom", I'm thinking that it should be clearly stated in the API documentation: the place to look for any user of an API function. So, I hope that given enough time there won't be a need to read all of our code in the code base in order to be able to extend LedgerSMB.

Regards,

Erik.

The return statement is designed specifically to be handy when returning
nothing in list or scalar context. I suppose the basic example is:

sub filter_things {
if ( $it eq "good" ) {
return $it;
}
return;
}

When you use such a function to build up an array the bare return returns
an empty list. So you are not building large arrays with useless elements,
so don't have to skip over them in processing, and so this works:

if_or_while ( @array ) { more_to_do(); }

because you did not return a list with one undefined element but returned
an empty list.

If you don't use this basic idiom, i.e. the duplex nature of bare return,
then there is little point to using RequireFinalReturn. This what a final
return is meant to do.

> I therefore propose that we do not enforce the
> ProhibitExplicitReturnUndef policy and will add a comment to the tests

If we are not enforcing the policy we can just drop it from our suite.

> Thanks to k-man and dcg_mx for initiating and sharing the discussion
> with Matt Trout.

Thanks,
Rob

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Ledger-smb-devel mailing list
Ledger-smb-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ledger-smb-devel

Bye,

Erik.

http://efficito.com -- Hosted accounting and ERP.

Robust and Flexible. No vendor lock-in.