Re: [COMMITTERS] pgsql: Clean up Perl code according to perlcritic

Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> writes:

> On 03/28/2017 05:23 AM, Dagfinn Ilmari Mannsåker wrote:
>> + @opts = grep { !/\$\(/ && /^--/ }
>> + map { (my $x = $_) =~ s/\Q$(top_builddir)\E/\"$topdir\"/; $x;}
>> + split(/\s+/, $1);
>>
>
> The use of this lexical $x variable seems entirely pointless and
> obfuscatory. If perlcritic doesn't like it without then that's another
> black mark against it IMNSHO.

The lexical copy is not strictly necessary in this case, because the
values we're mapping over are mutable temporaries returned by split().

An alternative form would be:

@opts = grep { !/\$\(/ && /^--/ }
map { s/\Q$(top_builddir)\E/\"$topdir\"/; $_ }
split(/\s+/, $1);

Perlcritic complains about that, though:

src/tools/msvc/vcregress.pl: Don't modify $_ in list functions at line
524, column 4. See page 114 of PBP. (Severity: 5)

This for two reasons:

1) If we were mapping over an array varible it would modify the original
values in-place (since $_ is an alias, not a copy).

2) If the list were to contain read-only items it'd throw a
"Modification of a read-only value attempted" exception.

The /r modifier was introduced in perl 5.14 exactly for this reason: it
makes the substitution non-destructive and returns the modified string
instead. However, we still support perls as ancient as 5.8, so we can't
use that.

- ilmari

--
- Twitter seems more influential [than blogs] in the 'gets reported in
the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
to a mainstream media article. - Calle Dybedahl