Re: [PATCHv5 03/17] gitweb/lib - Very simple file based cache

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 6, 2010, 22:44 UTC
Message-ID: <AANLkTik1RzGno+TsZe5OZSU7SeJOhDKWFCZYmrFhH6SZ@mail.gmail.com>
In-Reply-To: <AANLkTinPCOfknoN4aO_EdPwKorRiM6NU6ep0z_nnNug8@mail.gmail.com>

On Wed, Oct 6, 2010 at 22:41, Thomas Adam <thomas@xteddy.org> wrote:

Show 16 quoted lines

> On 6 October 2010 23:01, Jakub Narebski <jnareb@gmail.com> wrote:
>> +# creates get_depth() and set_depth($depth) etc. methods
>> +foreach my $i (qw(depth root namespace)) {
>> +       my $field = $i;
>> +       no strict 'refs';
>
> For each item, you'll set "no strict refs"?  This might be better off
> outside the loop.  It's still scoped appropriately inside the
> subroutine.
>
>> +       my $file = $self->path_to_key($key);
>> +       return undef unless (defined $file && -f $file);
>
> PBP (Perl Best Practises) will tell you that explicitly returning
> undef is discouraged -- "undef" should be reserved for those errors
> you cannot handle, not ones you don't want to.

[...]

>> +       return unless (defined $key && defined $data);
>
> return what?

false. You're right that "return undef;" is bad style, but "return;" is what you should use instead.

Then you'll get undef in scalar context, and an empty list in list context. With "return undef" you'll always get an undef, so:

    sub blah { retur undef }
    my (@foo) = blah();

Will make @foo = (undef), which'll make it evaluate to true in scalar context since there's an item in the array, and give you a useless array item.