> On 21 Mar 2017, at 19:20, David Steele <da...@pgmasters.net> 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 <peter.eisentr...@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
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
