Re: perlcritic - Mailing list pgsql-hackers
From | ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) |
---|---|
Subject | Re: perlcritic |
Date | |
Msg-id | d8jd1d2x22p.fsf@dalvik.ping.uio.no Whole thread Raw |
In response to | Re: perlcritic (Peter Eisentraut <peter.eisentraut@2ndquadrant.com>) |
List | pgsql-hackers |
Peter Eisentraut <peter.eisentraut@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.
pgsql-hackers by date: