Peter Eisentraut <peter.eisentr...@2ndquadrant.com> writes:
> On 3/1/17 11:21, Dagfinn Ilmari Mannsåker wrote:
>> diff --git a/src/pl/plperl/plc_perlboot.pl b/src/pl/plperl/plc_perlboot.pl
>> index 292c9101c9..b4212f5ab2 100644
>> --- a/src/pl/plperl/plc_perlboot.pl
>> +++ b/src/pl/plperl/plc_perlboot.pl
>> @@ -81,18 +81,15 @@ sub ::encode_array_constructor
>> } sort keys %$imports;
>> $BEGIN &&= "BEGIN { $BEGIN }";
>>
>> - return qq[ package main; sub { $BEGIN $prolog $src } ];
>> + # default no strict and no warnings
>> + return qq[ package main; sub { no strict; no warnings; $BEGIN
>> $prolog $src } ];
>> }
>>
>> sub mkfunc
>> {
>> - ## no critic (ProhibitNoStrict, ProhibitStringyEval);
>> - no strict; # default to no strict for the eval
>> - no warnings; # default to no warnings for the eval
>> - my $ret = eval(mkfuncsrc(@_));
>> + my $ret = eval(mkfuncsrc(@_)); ## no critic
>> (ProhibitStringyEval);
>> $@ =~ s/\(eval \d+\) //g if $@;
>> return $ret;
>> - ## use critic
>> }
>>
>> 1;
>
> I have no idea what this code does or how to test it, so I didn't touch it.
This code compiles a string of perl source into a subroutine reference.
It's is called by plperl_create_sub() in src/pl/plperl/plperl.c, which
is called whenever a plperl function needs to be compiled, i.e. during
CREATE FUNCTION (unless check_function_bodies is off) and when the
function is executed and the compiled form is not already cached in
plperl_proc_hash.
The change reduces the scope of the stricture and warning disablement to
just the compiled code, instead of the surrounding compiling code too.
Putting them inside the sub block has no runtime overhead, since they're
compile-time directives, not runtime statements.
It can be tested by creating a plperl function with a construct that
would fall foul of warnings or strictures, which
src/pl/plperl/sql/plperl_elog.sql does.
>> diff --git a/src/tools/msvc/gendef.pl b/src/tools/msvc/gendef.pl
>> index 64227c2dce..e2653f11d8 100644
>> --- a/src/tools/msvc/gendef.pl
>> +++ b/src/tools/msvc/gendef.pl
>> @@ -174,7 +174,7 @@ sub usage
>>
>> my %def = ();
>>
>> -while (<$ARGV[0]/*.obj>) ## no critic (RequireGlobFunction);
>> +while (glob($ARGV[0]/*.obj))
>> {
>> my $objfile = $_;
>> my $symfile = $objfile;
>
> I think what this code is meant to do might be better written as a
> foreach loop. Again, can't test it.
glob("...") is exactly equivalent to <...> (except when <...> parses as
readline, which is why Perl::Critic complains).
Writing it as 'for my $objfile (glob("$ARGV[0]/*.obj")) { ... }' would
be neater, I agree.
>> difff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
>> index a6b24b5348..51d6a28953 100755
>> --- a/src/tools/pgindent/pgindent
>> +++ b/src/tools/pgindent/pgindent
>> @@ -159,8 +159,7 @@ sub process_exclude
>> while (my $line = <$eh>)
>> {
>> chomp $line;
>> - my $rgx;
>> - eval " \$rgx = qr!$line!;"; ## no critic
>> (ProhibitStringyEval);
>> + my $rgx = eval { qr!$line! };
>> @files = grep { $_ !~ /$rgx/ } @files if $rgx;
>> }
>> close($eh);
>
> After further thinking, I changed this to just
>
> my $rgx = qr!$line!;
>
> which works just fine.
That changes the behaviour from silently skipping invalid regular
expressions in the exclude file to dying on encountering one. That
might be desirable, but should be done deliberately.
>> @@ -435,7 +434,8 @@ sub diff
>>
>> sub run_build
>> {
>> - eval "use LWP::Simple;"; ## no critic (ProhibitStringyEval);
>> + require LWP::Simple;
>> + LWP::Simple->import(qw(getstore is_success));
>>
>> my $code_base = shift || '.';
>> my $save_dir = getcwd();
>
> I think this is mean to not fail compilation if you don't have that
> module, so I left it as is.
Yes, it's using string eval to defer the compilation of the "use"
statement to runtime. The require+import does exactly the same thing,
since they are run-time already, so won't be called until run_build is.
While looking at this again, I realised that the 'do' statement in
src/tools/msvc/install.pl will break on the upcoming perl 5.26, which
doesn't include '.' in @INC (the search path for 'require' and 'do') by
default.
if (-e "src/tools/msvc/buildenv.pl")
{
do "src/tools/msvc/buildenv.pl";
}
Attached is a final patch with the above changes, which I think should
be applied before this can be considered complete.
>From 1d388d13d572912df2faa7d1c4004a635f956306 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilm...@ilmari.org>
Date: Wed, 1 Mar 2017 15:32:45 +0000
Subject: [PATCH] Fix most remaining perlcritic exceptions
The ProhibitStringyEval one is unavoidable when compiling plperl
functions at runtime.
The RequireFilenameMatchesPackage ones would require reworking the
plperl build process more drastically.
---
src/pl/plperl/plc_perlboot.pl | 9 +++------
src/tools/msvc/gendef.pl | 6 ++----
src/tools/msvc/install.pl | 4 ++--
src/tools/pgindent/pgindent | 3 ++-
4 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/src/pl/plperl/plc_perlboot.pl b/src/pl/plperl/plc_perlboot.pl
index 292c9101c9..b4212f5ab2 100644
--- a/src/pl/plperl/plc_perlboot.pl
+++ b/src/pl/plperl/plc_perlboot.pl
@@ -81,18 +81,15 @@ sub ::encode_array_constructor
} sort keys %$imports;
$BEGIN &&= "BEGIN { $BEGIN }";
- return qq[ package main; sub { $BEGIN $prolog $src } ];
+ # default no strict and no warnings
+ return qq[ package main; sub { no strict; no warnings; $BEGIN $prolog $src } ];
}
sub mkfunc
{
- ## no critic (ProhibitNoStrict, ProhibitStringyEval);
- no strict; # default to no strict for the eval
- no warnings; # default to no warnings for the eval
- my $ret = eval(mkfuncsrc(@_));
+ my $ret = eval(mkfuncsrc(@_)); ## no critic (ProhibitStringyEval);
$@ =~ s/\(eval \d+\) //g if $@;
return $ret;
- ## use critic
}
1;
diff --git a/src/tools/msvc/gendef.pl b/src/tools/msvc/gendef.pl
index 64227c2dce..c5c7f9c25f 100644
--- a/src/tools/msvc/gendef.pl
+++ b/src/tools/msvc/gendef.pl
@@ -174,11 +174,9 @@ sub usage
my %def = ();
-while (<$ARGV[0]/*.obj>) ## no critic (RequireGlobFunction);
+for my $objfile (glob("$ARGV[0]/*.obj"))
{
- my $objfile = $_;
- my $symfile = $objfile;
- $symfile =~ s/\.obj$/.sym/i;
+ (my $symfile = $objfile) =~ s/\.obj$/.sym/i;
dumpsyms($objfile, $symfile);
print ".";
extract_syms($symfile, \%def);
diff --git a/src/tools/msvc/install.pl b/src/tools/msvc/install.pl
index b2d7f9e040..f4bd0d0e75 100755
--- a/src/tools/msvc/install.pl
+++ b/src/tools/msvc/install.pl
@@ -12,9 +12,9 @@
# it should contain lines like:
# $ENV{PATH} = "c:/path/to/bison/bin;$ENV{PATH}";
-if (-e "src/tools/msvc/buildenv.pl")
+if (-e "./src/tools/msvc/buildenv.pl")
{
- do "src/tools/msvc/buildenv.pl";
+ do "./src/tools/msvc/buildenv.pl";
}
elsif (-e "./buildenv.pl")
{
diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index 0f3a1ba69a..3d24a092a8 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -434,7 +434,8 @@ sub diff
sub run_build
{
- eval "use LWP::Simple;"; ## no critic (ProhibitStringyEval);
+ require LWP::Simple;
+ LWP::Simple->import(qw(getstore is_success));
my $code_base = shift || '.';
my $save_dir = getcwd();
--
2.11.0
--
"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
