Thread: Re: [COMMITTERS] pgsql: Clean up Perl code according to perlcritic
Peter Eisentraut <peter_e@gmx.net> writes:
> Clean up Perl code according to perlcritic
This seems to have broken the regression tests (specifically, dblink)
on at least some of the Windows buildfarm critters. It looks like
something got changed in the behavior of build-tree path expansion:
this in the input/paths.source file
CREATE FUNCTION putenv(text) RETURNS void AS '@libdir@/regress@DLSUFFIX@', 'regress_putenv' LANGUAGE C STRICT;
results in
CREATE FUNCTION putenv(text) RETURNS void RETURNS void AS
'C:/buildfarm/HEAD/pgsql.build/contrib/dblink/$(top_builddir)/src/test/regress/regress.dll','regress_putenv'
LANGUAGEC STRICT;
+ ERROR: could not access file
"C:/buildfarm/HEAD/pgsql.build/contrib/dblink/$(top_builddir)/src/test/regress/regress.dll":No such file or directory
which is surely not what it ought to be.
regards, tom lane
I wrote:
> Peter Eisentraut <peter_e@gmx.net> writes:
>> Clean up Perl code according to perlcritic
> This seems to have broken the regression tests (specifically, dblink)
> on at least some of the Windows buildfarm critters.
I'm hardly a Perl expert, but it looks to me like the culprit is this
hunk in vcregress.pl:
@@ -521,8 +521,9 @@ sub fetchRegressOpts # an unhandled variable reference. Ignore anything that isn't an
# option starting with "--". @opts = grep {
- s/\Q$(top_builddir)\E/\"$topdir\"/;
- $_ !~ /\$\(/ && $_ =~ /^--/
+ my $x = $_;
+ $x =~ s/\Q$(top_builddir)\E/\"$topdir\"/;
+ $x !~ /\$\(/ && $x =~ /^--/ } split(/\s+/, $1); } if ($m =~ /^\s*ENCODING\s*=\s*(\S+)/m)
The first line in that block is actually intending to modify the value
it's inspecting, and perlcritic's "improved" version carefully removes
the side-effect.
No doubt there are cleaner ways to do that (the comments in "man perlfunc"
about this coding technique are not positive), but this way is not
cleaner, it is broken.
regards, tom lane
Re: [COMMITTERS] pgsql: Clean up Perl code according to perlcritic
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
Tom Lane <tgl@sss.pgh.pa.us> writes:
> I wrote:
>> Peter Eisentraut <peter_e@gmx.net> writes:
>>> Clean up Perl code according to perlcritic
>
>> This seems to have broken the regression tests (specifically, dblink)
>> on at least some of the Windows buildfarm critters.
>
> I'm hardly a Perl expert, but it looks to me like the culprit is this
> hunk in vcregress.pl:
>
> @@ -521,8 +521,9 @@ sub fetchRegressOpts
> # an unhandled variable reference. Ignore anything that isn't an
> # option starting with "--".
> @opts = grep {
> - s/\Q$(top_builddir)\E/\"$topdir\"/;
> - $_ !~ /\$\(/ && $_ =~ /^--/
> + my $x = $_;
> + $x =~ s/\Q$(top_builddir)\E/\"$topdir\"/;
> + $x !~ /\$\(/ && $x =~ /^--/
> } split(/\s+/, $1);
> }
> if ($m =~ /^\s*ENCODING\s*=\s*(\S+)/m)
>
> The first line in that block is actually intending to modify the value
> it's inspecting, and perlcritic's "improved" version carefully removes
> the side-effect.
>
> No doubt there are cleaner ways to do that (the comments in "man perlfunc"
> about this coding technique are not positive), but this way is not
> cleaner, it is broken.
I suggest splitting the substitution and filtering part into separate
map and grep calls, for clarity. The substituion is crying out for the
/r regex modifier to avoid the local variable, but that's only available
since perl 5.14.
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index d9367f8..cf93a60 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -520,11 +520,9 @@ sub fetchRegressOpts # Substitute known Makefile variables, then ignore options that retain
# an unhandled variable reference. Ignore anything that isn't an # option starting with "--".
- @opts = grep {
- my $x = $_;
- $x =~ s/\Q$(top_builddir)\E/\"$topdir\"/;
- $x !~ /\$\(/ && $x =~ /^--/
- } split(/\s+/, $1);
+ @opts = grep { !/\$\(/ && /^--/ }
+ map { (my $x = $_) =~ s/\Q$(top_builddir)\E/\"$topdir\"/; $x;}
+ split(/\s+/, $1); } if ($m =~ /^\s*ENCODING\s*=\s*(\S+)/m) {
- ilmari
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live withthe consequences of."
--Skud's Meta-Law
On 03/27/2017 08:58 PM, Tom Lane wrote:
> I wrote:
>> Peter Eisentraut <peter_e@gmx.net> writes:
>>> Clean up Perl code according to perlcritic
>> This seems to have broken the regression tests (specifically, dblink)
>> on at least some of the Windows buildfarm critters.
> I'm hardly a Perl expert, but it looks to me like the culprit is this
> hunk in vcregress.pl:
>
> @@ -521,8 +521,9 @@ sub fetchRegressOpts
> # an unhandled variable reference. Ignore anything that isn't an
> # option starting with "--".
> @opts = grep {
> - s/\Q$(top_builddir)\E/\"$topdir\"/;
> - $_ !~ /\$\(/ && $_ =~ /^--/
> + my $x = $_;
> + $x =~ s/\Q$(top_builddir)\E/\"$topdir\"/;
> + $x !~ /\$\(/ && $x =~ /^--/
> } split(/\s+/, $1);
> }
> if ($m =~ /^\s*ENCODING\s*=\s*(\S+)/m)
>
> The first line in that block is actually intending to modify the value
> it's inspecting, and perlcritic's "improved" version carefully removes
> the side-effect.
>
> No doubt there are cleaner ways to do that (the comments in "man perlfunc"
> about this coding technique are not positive), but this way is not
> cleaner, it is broken.
>
>
I would try something like this:
@opts = grep { $_ !~ /\$\(/ && $_ =~ /^--/ } map { s/\Q$(top_builddir)\E/\"$topdir\"/; } split(/\s+/,
$1);
but I don't have time to test it before I leave for pgconfUS.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [COMMITTERS] pgsql: Clean up Perl code according to perlcritic
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
Andrew Dunstan <andrew.dunstan@2ndquadrant.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