Re: perlcritic

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)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.