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'
LANGUAGE C 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 03/27/2017 08:23 AM, Peter Eisentraut wrote:

Clean up Perl code according to perlcritic

Fix all perlcritic warnings of severity level 5, except in
src/backend/utils/Gen_dummy_probes.pl, which is automatically generated.

Reviewed-by: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>

This contains at least one non-cosmetic change and the MSVC buildfarm
members have reacted accordingly:

            @opts = grep {
    -           s/\Q$(top_builddir)\E/\"$topdir\"/;
    -           $_ !~ /\$\(/ && $_ =~ /^--/
    +           my $x = $_;
    +           $x =~ s/\Q$(top_builddir)\E/\"$topdir\"/;
    +           $x !~ /\$\(/ && $x =~ /^--/

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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 with
the consequences of." -- Skud's Meta-Law

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

I would try something like this:

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

That map is not going to work: it'll modify the values returned by
split(), but s/// (without the /r modifier, which was added in 5.14)
returns the number of substitutions made, not the modified string.

--
"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 with
the consequences of." -- Skud's Meta-Law

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 03/28/2017 07:31 AM, Dagfinn Ilmari Manns�ker wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

I would try something like this:

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

That map is not going to work: it'll modify the values returned by
split(), but s/// (without the /r modifier, which was added in 5.14)
returns the number of substitutions made, not the modified string.

Oh. Bleah.

OK. well, we should note that in comments then.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 3/28/17 05:23, Dagfinn Ilmari Manns�ker wrote:

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)
{

Committed that way.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers