Re: [HACKERS] perlcritic
Peter Eisentraut 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?= Date: Wed, 1 Mar 2017 15:32:45 + Subject: [PATCH] Fix most remaining perlcritic exceptions The ProhibitStringyEval one is unavoidable when compili
Re: [HACKERS] perlcritic
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] perlcritic
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] perlcritic
> On 21 Mar 2017, at 19:20, David Steele wrote: > > On 3/6/17 12:02 PM, Dagfinn Ilmari Mannsåker wrote: >> ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes: >> >>> Hi Peter, >>> >>> Peter Eisentraut 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) = ); -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 perlcritic-with-review-comments.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] perlcritic
> On 21 Mar 2017, at 19:20, David Steele wrote: > > On 3/6/17 12:02 PM, Dagfinn Ilmari Mannsåker wrote: >> ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes: >> >>> Hi Peter, >>> >>> Peter Eisentraut 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] perlcritic
Hi Daniel, On 3/6/17 12:02 PM, Dagfinn Ilmari Mannsåker wrote: ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes: Hi Peter, Peter Eisentraut 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 da...@pgmasters.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] perlcritic
ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes: > Hi Peter, > > Peter Eisentraut 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?= Date: Wed, 1 Mar 2017 15:32:45 + 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
Re: [HACKERS] perlcritic
Hi Peter, Peter Eisentraut 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?= Date: Wed, 1 Mar 2017 15:32:45 + 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
Re: [HACKERS] perlcritic
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 From af08d7e1b7a947a3f94bb1cef7508c3f926cc35a Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Wed, 4 Jan 2017 12:00:00 -0500 Subject: [PATCH v2 1/2] Clean up Perl code according to perlcritic severity level 5 --- contrib/intarray/bench/create_test.pl| 20 +-- doc/src/sgml/generate-errcodes-table.pl | 2 +- doc/src/sgml/mk_feature_tables.pl| 12 +- src/backend/catalog/Catalog.pm | 8 +- src/backend/catalog/genbki.pl| 64 - src/backend/parser/check_keywords.pl | 30 ++--- src/backend/storage/lmgr/generate-lwlocknames.pl | 30 ++--- src/backend/utils/Gen_fmgrtab.pl | 32 ++--- src/backend/utils/generate-errcodes.pl | 2 +- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 26 ++-- src/bin/pg_ctl/t/001_start_stop.pl | 14 +- src/bin/psql/create_help.pl | 28 ++-- src/interfaces/ecpg/preproc/check_rules.pl | 12 +- src/interfaces/libpq/test/regress.pl | 10 +- src/pl/plperl/plc_perlboot.pl| 4 +- src/pl/plperl/plc_trusted.pl | 2 +- src/pl/plperl/text2macro.pl | 8 +- src/pl/plpgsql/src/generate-plerrcodes.pl| 2 +- src/pl/plpython/generate-spiexceptions.pl| 2 +- src/pl/tcl/generate-pltclerrcodes.pl | 2 +- src/test/locale/sort-test.pl | 4 +- src/test/perl/PostgresNode.pm| 8 +- src/test/perl/TestLib.pm | 16 +-- src/test/ssl/ServerSetup.pm | 48 +++ src/tools/fix-old-flex-code.pl | 4 +- src/tools/msvc/Install.pm| 10 +- src/tools/msvc/Mkvcbuild.pm | 2 +- src/tools/msvc/Project.pm| 28 ++-- src/tools/msvc/Solution.pm | 162 +++ src/tools/msvc/build.pl | 8 +- src/tools/msvc/builddoc.pl | 2 +- src/tools/msvc/gendef.pl | 18 +-- src/tools/msvc/install.pl| 4 +- src/tools/msvc/mkvcbuild.pl | 4 +- src/tools/msvc/pgbison.pl| 4 +- src/tools/msvc/pgflex.pl | 12 +- src/tools/msvc/vcregress.pl | 19 +-- src/tools/pginclude/pgcheckdefines | 32 ++--- src/tools/pgindent/pgindent | 4 +- src/tools/version_stamp.pl | 6 +- src/tools/win32tzlist.pl | 6 +- 41 files changed, 356 insertions(+), 355 deletions(-) diff --git a/contrib/intarray/bench/create_test.pl b/contrib/intarray/bench/create_test.pl index 1323b31e4d..f3262df05b 100755 --- a/contrib/intarray/bench/create_test.pl +++ b/contrib/intarray/bench/create_test.pl @@ -15,8 +15,8 @@ EOT -open(MSG, ">message.tmp") || die; -open(MAP, ">message_section_map.tmp") || die; +open(my $msg, '>', "message.tmp") || die; +open(my $map, '>', "message_section_map.tmp") || die; srand(1); @@ -42,16 +42,16 @@ } if ($#sect < 0 || rand() < 0.1) { - print MSG "$i\t\\N\n"; + print $msg "$i\t\\N\n"; } else { - print MSG "$i\t{" . join(',', @sect) . "}\n"; - map { print MAP "$i\t$_\n" } @sect; + print $msg "$i\t{" . join(',',
Re: [HACKERS] perlcritic
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] perlcritic
On Tue, Sep 01, 2015 at 11:26:27AM -0400, Robert Haas wrote: > On Tue, Sep 1, 2015 at 9:58 AM, Andrew Dunstan 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] perlcritic
On Tue, Sep 1, 2015 at 11:58 AM, Mike Blackwell 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] perlcritic
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. ^_^
Re: [HACKERS] perlcritic
On Tue, Sep 01, 2015 at 11:26:27AM -0400, Robert Haas wrote: > On Tue, Sep 1, 2015 at 9:58 AM, Andrew Dunstan 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 http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] perlcritic
On Tue, Sep 1, 2015 at 9:58 AM, Andrew Dunstan 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] perlcritic
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] perlcritic
On Sep 1, 2015 6:25 AM, "Michael Paquier" wrote: > > On Tue, Sep 1, 2015 at 12: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). > > 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. It seems like something we want to do at some point, so we're going to have to take the pain at some point. We might want to wait until after 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. /Magnus
Re: [HACKERS] perlcritic
On Tue, Sep 1, 2015 at 12: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). 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers