Alvaro Herrera <alvhe...@alvh.no-ip.org> writes:
> On 2021-Jul-28, David Rowley wrote:
>
>> 0003: Is a tidy up patch to make the 'includes' field an array rather
>> than a string
>
> In this one, you can avoid turning one line into four with map,
>
> - $p->AddIncludeDir($pl_proj->{includes});
> + foreach my $inc (@{ $pl_proj->{includes} })
> + {
> + $p->AddIncludeDir($inc);
> + }
>
> Instead of that you can do something like this:
>
> + map { $p->AddIncludeDir($_); } @{$pl_proj->{includes}};
map (and grep) should never be used void context for side effects. Our
perlcritic policy doesn't currently forbid that, but it should (and
there is one violation of that in contrib/intarray). I'll submit a
patch for that separately.
The acceptable one-liner version would be a postfix for loop:
$p->AddIncludeDir($_) for @{$pl_proj->{includes}};
>> 0004: Adds code to check for duplicate references and libraries before
>> adding new ones of the same name to the project.
>
> I think using the return value of grep as a boolean is confusing. It
> seems more legible to compare to 0. So instead of this:
>
> + if (! grep { $_ eq $ref} @{ $self->{references} })
> + {
> + push @{ $self->{references} }, $ref;
> + }
>
> use something like:
>
> + if (grep { $_ eq $ref} @{ $self->{references} } == 0)
I disagree. Using grep in boolean context is perfectly idiomatic perl.
What would be more idiomatic is List::Util::any, but that's not availble
without upgrading List::Util from CPAN on Perls older than 5.20, so we
can't use that.
- ilmari
