Thread: perlcritic
We now have 80+ Perl files in our tree, and it's growing. Some of those
files were originally written for Perl 4, and the coding styles and
quality are quite, uh, divergent. So I figured it's time to clean up
that code a bit. I ran perlcritic over the tree and cleaned up all the
warnings at level 5 (the default, least severe).
Testing guidelines:
- Many files are part of the regular build or test process.
- msvc files need to be tested separately. I tested as best as I could
on a non-Windows system.
- There are a couple of one-offs in contrib and src/test that need to be
run manually.
- The stuff under utils/mb/Unicode/ has a makefile that is not part of
the normal build process. I'll send in a few more patches to that in a
separate message that should help testing.
To install perlcritic, run
cpan -i Perl::Critic
and then run
perlcritic .
at the top of the tree (or a subdirectory).
Attachment
On Tue, Sep 1, 2015 at 12:57 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > We now have 80+ Perl files in our tree, and it's growing. Some of those > files were originally written for Perl 4, and the coding styles and > quality are quite, uh, divergent. So I figured it's time to clean up > that code a bit. I ran perlcritic over the tree and cleaned up all the > warnings at level 5 (the default, least severe). Do you think we should be concerned about the increased difficulty to backpatch fixes if this patch is applied? I personally think that's fine to do this cleanup on HEAD only, still others may have a different opinion. > Testing guidelines: > - msvc files need to be tested separately. I tested as best as I could > on a non-Windows system. And tested on Windows, I am not seeing failures. -- Michael
<p dir="ltr"><br /> On Sep 1, 2015 6:25 AM, "Michael Paquier" <<a href="mailto:michael.paquier@gmail.com">michael.paquier@gmail.com</a>>wrote:<br /> ><br /> > On Tue, Sep 1, 2015at 12:57 PM, Peter Eisentraut <<a href="mailto:peter_e@gmx.net">peter_e@gmx.net</a>> wrote:<br /> > > Wenow have 80+ Perl files in our tree, and it's growing. Some of those<br /> > > files were originally written forPerl 4, and the coding styles and<br /> > > quality are quite, uh, divergent. So I figured it's time to clean up<br/> > > that code a bit. I ran perlcritic over the tree and cleaned up all the<br /> > > warnings at level5 (the default, least severe).<br /> ><br /> > Do you think we should be concerned about the increased difficultyto<br /> > backpatch fixes if this patch is applied? I personally think that's<br /> > fine to do this cleanupon HEAD only, still others may have a<br /> > different opinion.<br /><p dir="ltr">It seems like something we wantto do at some point, so we're going to have to take the pain at some point. <p dir="ltr">We might want to wait untilafter we get 9.5 to rc as there's likely to be more changes then, but I don't see any point in delaying it beyond that.<p dir="ltr">/Magnus <br />
On 08/31/2015 11:57 PM, Peter Eisentraut wrote:
> We now have 80+ Perl files in our tree, and it's growing. Some of those
> files were originally written for Perl 4, and the coding styles and
> quality are quite, uh, divergent. So I figured it's time to clean up
> that code a bit. I ran perlcritic over the tree and cleaned up all the
> warnings at level 5 (the default, least severe).
I don't object to this. Forcing strict mode is good, and I think I
stopped using bareword file handles around 17 years ago. OTOH, I don't
care that much about the two argument form of open(), and I doubt we
gain a heck of a lot by changing it to the three argument form. It seems
to me more a matter of stylistic preference than any significant
technical improvement. In some cases it arguably leads to less clarity,
e.g. this doesn't seem to add clarity:
- open $fh, "./$tmp |" or die; + open $fh, '<', "./$tmp |" or die;
Note also that in some cases all that's happened is that it's added
comments so that future percritic runs will ignore what it's complaining
about. We should look at those cases and annotate them (either we're
happy the way it is or we should fix them)
In pgindent, there are a couple of uses of eval that are mine
originally. At least one should be able to be replaced, thus:
require LWP::Simple; LWP::Simple->import('getstore');
I'd have to look at the other one more closely.
cheers
andrew
On Tue, Sep 1, 2015 at 9:58 AM, Andrew Dunstan <andrew@dunslane.net> wrote: > On 08/31/2015 11:57 PM, Peter Eisentraut wrote: >> We now have 80+ Perl files in our tree, and it's growing. Some of those >> files were originally written for Perl 4, and the coding styles and >> quality are quite, uh, divergent. So I figured it's time to clean up >> that code a bit. I ran perlcritic over the tree and cleaned up all the >> warnings at level 5 (the default, least severe). > > I don't object to this. Forcing strict mode is good, and I think I stopped > using bareword file handles around 17 years ago. FWIW, I think perlcritic is both useless and annoying. I've always used bareword file handles, and I don't really see what the problem with it is, especially in very short script files. And what's wrong with two-argument form of open, if the path is a constant rather than possibly-tainted user input? Perl advertises that TMTOWTDI, and then perlcritic complains about which one you picked, mostly AFAICS for no particularly compelling reason. So I'm pretty meh about this whole exercise, especially if we follow it up by cleaning up the lower levels of warnings which, from what I can see, are unnecessary pedantry on top of unnecessary pedantry. But I suspect I'm in the minority here, so feel free to ignore me. (I certainly do agree that use strict and use warnings are a good thing to use everywhere. It's just perlcritic I dislike.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Sep 01, 2015 at 11:26:27AM -0400, Robert Haas wrote: > On Tue, Sep 1, 2015 at 9:58 AM, Andrew Dunstan <andrew@dunslane.net> wrote: > > On 08/31/2015 11:57 PM, Peter Eisentraut wrote: > >> We now have 80+ Perl files in our tree, and it's growing. Some > >> of those files were originally written for Perl 4, and the coding > >> styles and quality are quite, uh, divergent. So I figured it's > >> time to clean up that code a bit. I ran perlcritic over the tree > >> and cleaned up all the warnings at level 5 (the default, least > >> severe). > > > > I don't object to this. Forcing strict mode is good, and I think I > > stopped using bareword file handles around 17 years ago. > > FWIW, I think perlcritic is both useless and annoying. I've always > used bareword file handles, and I don't really see what the problem > with it is, especially in very short script files. And what's wrong > with two-argument form of open, if the path is a constant rather > than possibly-tainted user input? Perl advertises that TMTOWTDI, > and then perlcritic complains about which one you picked, mostly > AFAICS for no particularly compelling reason. So I'm pretty meh > about this whole exercise, especially if we follow it up by cleaning > up the lower levels of warnings which, from what I can see, are > unnecessary pedantry on top of unnecessary pedantry. > > But I suspect I'm in the minority here, so feel free to ignore me. > (I certainly do agree that use strict and use warnings are a good > thing to use everywhere. It's just perlcritic I dislike.) I believe there are ways to get perlcritic to keep quiet about things we don't find relevant. Maybe that's a better way to use it. Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
David wrote:
I believe there are ways to get perlcritic to keep quiet about things
we don't find relevant. Maybe that's a better way to use it.
There are indeed. A .perlcriticrc file can suppress (or add) either individual rules or groups of rules. I use one to ignore the ones I disagree with, along with the comment form to ignore specific cases.
I see perlcritic as functioning for me along the same lines as a style guide, giving a consistency that helps with long term maintainability.
It also helps keep me from straying into golf. ^_^
On Tue, Sep 1, 2015 at 11:58 AM, Mike Blackwell <mike.blackwell@rrd.com> wrote: > David wrote: >> I believe there are ways to get perlcritic to keep quiet about things >> we don't find relevant. Maybe that's a better way to use it. > > There are indeed. A .perlcriticrc file can suppress (or add) either > individual rules or groups of rules. I use one to ignore the ones I > disagree with, along with the comment form to ignore specific cases. Well, then we'd have to agree on which rules have any value; it will probably be impossible to get consensus on that. My suggestion for a .perlcriticrc file will be one that ignores all of the rules and, if there's a way to do it, causes perlcritic to uninstall itself and leave behind a note apologizing for its existence. :-) In all seriousness, I'm totally fine with trying to create more stylistic consistency among our Perl scripts, and if Peter finds perlcritic a helpful way to get there, that's fair enough. But for myself, I am an inveterate perlcriticcritic. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Sep 01, 2015 at 11:26:27AM -0400, Robert Haas wrote: > On Tue, Sep 1, 2015 at 9:58 AM, Andrew Dunstan <andrew@dunslane.net> wrote: > > On 08/31/2015 11:57 PM, Peter Eisentraut wrote: > >> We now have 80+ Perl files in our tree, and it's growing. Some of those > >> files were originally written for Perl 4, and the coding styles and > >> quality are quite, uh, divergent. So I figured it's time to clean up > >> that code a bit. I ran perlcritic over the tree and cleaned up all the > >> warnings at level 5 (the default, least severe). > > > > I don't object to this. Forcing strict mode is good, and I think I stopped > > using bareword file handles around 17 years ago. > > FWIW, I think perlcritic is both useless and annoying. I've always > used bareword file handles, and I don't really see what the problem > with it is, especially in very short script files. A bareword file handle is a form of global variable, so the criticism is helpful for codebases large enough to make those a maintenance problem. It's important for any CPAN module, so I can understand perlcritic including it. Plenty of uses in our tree are fine. > And what's wrong > with two-argument form of open, if the path is a constant rather than > possibly-tainted user input? Perl advertises that TMTOWTDI, and then > perlcritic complains about which one you picked, mostly AFAICS for no > particularly compelling reason. So I'm pretty meh about this whole > exercise, especially if we follow it up by cleaning up the lower > levels of warnings which, from what I can see, are unnecessary > pedantry on top of unnecessary pedantry. > > But I suspect I'm in the minority here, so feel free to ignore me. (I > certainly do agree that use strict and use warnings are a good thing > to use everywhere. It's just perlcritic I dislike.) I agree with the rest of your message.
Hi, On 2015-08-31 23:57:25 -0400, Peter Eisentraut wrote: > We now have 80+ Perl files in our tree, and it's growing. Some of those > files were originally written for Perl 4, and the coding styles and > quality are quite, uh, divergent. So I figured it's time to clean up > that code a bit. I ran perlcritic over the tree and cleaned up all the > warnings at level 5 (the default, least severe). As far as I can see we haven't really come to any conclusion in this thread? Peter, where do you want to go from here? As this patch has been in waiting-for-author for a month, I'm marking it as returned-with-feedback. Greetings, Andres Freund
I posted this about 18 months ago but then ran out of steam. In the meantime, some people have been going around doing various Perl code cleanups in parts of the code, so it seems it makes sense to proceed with this. We use "use strict" everywhere now, so some of the original patch has gone away. Here is an updated patch. The testing instructions below still apply. Especially welcome would be ideas on how to address some of the places I have marked with ## no critic. On 8/31/15 23:57, Peter Eisentraut wrote: > We now have 80+ Perl files in our tree, and it's growing. Some of those [actually >=117 now] > files were originally written for Perl 4, and the coding styles and > quality are quite, uh, divergent. So I figured it's time to clean up > that code a bit. I ran perlcritic over the tree and cleaned up all the > warnings at level 5 (the default, least severe). > > Testing guidelines: > > - Many files are part of the regular build or test process. > > - msvc files need to be tested separately. I tested as best as I could > on a non-Windows system. > > - There are a couple of one-offs in contrib and src/test that need to be > run manually. > > - The stuff under utils/mb/Unicode/ [has already been cleaned up separately] > To install perlcritic, run > > cpan -i Perl::Critic > > and then run > > perlcritic . > > at the top of the tree (or a subdirectory). -- 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
Attachment
Hi Peter,
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> I posted this about 18 months ago but then ran out of steam. [ ] Here
> is an updated patch. The testing instructions below still apply.
> Especially welcome would be ideas on how to address some of the places
> I have marked with ## no critic.
Attached is a patch on top of yours that addresses all the ## no critic
annotations except RequireFilenameMatchesPackage, which can't be fixed
without more drastic reworking of the plperl build process.
Tested on perl 5.8.1 and 5.24.0 by configuring with --with-perl and
--enable-tap-tests followed by make check-world, and running pgindent
--build.
--
"A disappointingly low fraction of the human race is,
at any given time, on fire." - Stig Sandbeck Mathisen
From cdf3ca19cbbf03111243f9b39eb6f402f25b4502 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Wed, 1 Mar 2017 15:32:45 +0000
Subject: [PATCH] Fix most perlcritic exceptions
The RequireFilenameMatchesPackage ones would require reworking the
plperl build process more drastically.
---
src/pl/plperl/plc_perlboot.pl | 9 +++------
src/tools/msvc/gendef.pl | 2 +-
src/tools/pgindent/pgindent | 6 +++---
3 files changed, 7 insertions(+), 10 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..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;
diff --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);
@@ -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();
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
> Hi Peter,
>
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>
>> I posted this about 18 months ago but then ran out of steam. [ ] Here
>> is an updated patch. The testing instructions below still apply.
>> Especially welcome would be ideas on how to address some of the places
>> I have marked with ## no critic.
>
> Attached is a patch on top of yours that addresses all the ## no critic
> annotations except RequireFilenameMatchesPackage, which can't be fixed
> without more drastic reworking of the plperl build process.
>
> Tested on perl 5.8.1 and 5.24.0 by configuring with --with-perl and
> --enable-tap-tests followed by make check-world, and running pgindent
> --build.
Attached is an updated version of the patch, in which
src/tools/msvc/gendef.pl actually compiles. If someone on Windows could
test it, that would be great.
--
"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
From 2bbdd768bdbabe10e0af6b95d2d09d29095d3a8b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Wed, 1 Mar 2017 15:32:45 +0000
Subject: [PATCH] Fix most perlcritic exceptions
The RequireFilenameMatchesPackage ones would require reworking the
plperl build process more drastically.
---
src/pl/plperl/plc_perlboot.pl | 9 +++------
src/tools/msvc/gendef.pl | 2 +-
src/tools/pgindent/pgindent | 6 +++---
3 files changed, 7 insertions(+), 10 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..598699e6ea 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;
diff --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);
@@ -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();
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi Daniel, On 3/6/17 12:02 PM, Dagfinn Ilmari Mannsåker wrote: > ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes: > >> Hi Peter, >> >> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> >>> I posted this about 18 months ago but then ran out of steam. [ ] Here >>> is an updated patch. The testing instructions below still apply. >>> Especially welcome would be ideas on how to address some of the places >>> I have marked with ## no critic. >> >> Attached is a patch on top of yours that addresses all the ## no critic >> annotations except RequireFilenameMatchesPackage, which can't be fixed >> without more drastic reworking of the plperl build process. >> >> Tested on perl 5.8.1 and 5.24.0 by configuring with --with-perl and >> --enable-tap-tests followed by make check-world, and running pgindent >> --build. > > Attached is an updated version of the patch, in which > src/tools/msvc/gendef.pl actually compiles. If someone on Windows could > test it, that would be great. You are signed up to review this patch. Do you know when you'll have a chance to do that? Thanks, -- -David david@pgmasters.net
> On 21 Mar 2017, at 19:20, David Steele <david@pgmasters.net> wrote: > > On 3/6/17 12:02 PM, Dagfinn Ilmari Mannsåker wrote: >> ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes: >> >>> Hi Peter, >>> >>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >>> >>>> I posted this about 18 months ago but then ran out of steam. [ ] Here >>>> is an updated patch. The testing instructions below still apply. >>>> Especially welcome would be ideas on how to address some of the places >>>> I have marked with ## no critic. >>> >>> Attached is a patch on top of yours that addresses all the ## no critic >>> annotations except RequireFilenameMatchesPackage, which can't be fixed >>> without more drastic reworking of the plperl build process. >>> >>> Tested on perl 5.8.1 and 5.24.0 by configuring with --with-perl and >>> --enable-tap-tests followed by make check-world, and running pgindent >>> --build. >> >> Attached is an updated version of the patch, in which >> src/tools/msvc/gendef.pl actually compiles. If someone on Windows could >> test it, that would be great. > > You are signed up to review this patch. Do you know when you'll have a chance to do that? I have on my TODO for today or tomorrow to wrap that up. cheers ./daniel
> On 21 Mar 2017, at 19:20, David Steele <david@pgmasters.net> wrote: > > On 3/6/17 12:02 PM, Dagfinn Ilmari Mannsåker wrote: >> ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes: >> >>> Hi Peter, >>> >>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >>> >>>> I posted this about 18 months ago but then ran out of steam. [ ] Here >>>> is an updated patch. The testing instructions below still apply. >>>> Especially welcome would be ideas on how to address some of the places >>>> I have marked with ## no critic. >>> >>> Attached is a patch on top of yours that addresses all the ## no critic >>> annotations except RequireFilenameMatchesPackage, which can't be fixed >>> without more drastic reworking of the plperl build process. >>> >>> Tested on perl 5.8.1 and 5.24.0 by configuring with --with-perl and >>> --enable-tap-tests followed by make check-world, and running pgindent >>> --build. >> >> Attached is an updated version of the patch, in which >> src/tools/msvc/gendef.pl actually compiles. If someone on Windows could >> test it, that would be great. > > You are signed up to review this patch. Do you know when you'll have a chance to do that? Below is a review of the two patches attached to the commitfest entry: The v2-0001-Clean-up-Perl-code-according-to-perlcritic-severi.patch didn’t apply cleanly due to later commits, but the fixes to get there were trivial. The followup 0001-Fix-most-perlcritic-exceptions-v2.patch applied clean on top of that. The attached patch contains these two patches, rebased on top of current master, with the below small nitpicks. Since the original submission, most things have been addressed already, leaving this patch with mostly changing to three-close open. The no critic exceptions left are quite harmless: two cases of RequireFilenameMatchesPackage and one ProhibitStringyEval. All three could be fixed at the expense of complicating things without much (or any) benefit (as mentioned up-thread by Dagfinn Ilmari Mannsåker), so I’m fine with leaving them in. A few small nitpicks on the patch: ## In src/interfaces/libpq/test/regress.pl: -open(REGRESS_IN, "<", $regress_in) +open(my $regress_in_fh, "<", $regress_in) Reading and closing this file was still using REGRESS_IN, fixed in the attached updated patch. ## In src/test/locale/sort-test.pl: -open(INFILE, "<$ARGV[0]"); -chop(my (@words) = <INFILE>); -close(INFILE); +chop(my (@words) = <>); While this hunk does provide the same functionality due to the magic handling of ARGV in <>, it also carries the side “benefit” that arbitrary applications can be executed by using a | to read the output from a program: $ src/test/locale/sort-test.pl "rm README |" $ cat README cat: README: No such file or directory A silly example for sure, but since the intent of the patch is to apply best practices and safe practices, I’d argue that a normal three-clause open is safer here. The risk for misuse is very low, but it also makes the code less magic and more readable IMO. Reading the thread, most of the discussion was around the use of three-clause open instead of the older two-clause. Without diving into the arguments, there are a few places where we should use three-clause open, so simply applying it everywhere rather than on a case by case basis seems reasonable to me. Consistency across the codebase helps when reading the code. There is no measurable performance impact on the changes, and no user visible changes to functionality. With this applied, make check-world passes and perlcritic returns a clean run (except on the autogenerated Gen_dummy_probes.pl which is kept out of this work). The intent of the patch is to make the code consistent and readable, and it achieves that. I see no reason to not go ahead with these changes, if only to keep the codebase consistent with with modern Perl code is expected to look like. Given the nitpick nature of the comments, bumping status to ready for committer. cheers ./daniel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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.
> 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.
> diff --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.
> @@ -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.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/23/17 11:58, Daniel Gustafsson wrote: > Given the nitpick nature of the comments, bumping status to ready for > committer. Committed, with your changes. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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.