Re: cleaning perl code

On Wed, Apr 15, 2020 at 03:43:36PM -0400, Andrew Dunstan wrote:
> On 4/14/20 4:44 PM, Alvaro Herrera wrote:
> > On 2020-Apr-14, Andrew Dunstan wrote:
> >> One of the things that's a bit sad is that perlcritic doesn't generally
> >> let you apply policies to a given set of files or files matching some
> >> pattern. It would be nice, for instance, to be able to apply some
> >> additional standards to strategic library files like PostgresNode.pm,
> >> TestLib.pm and Catalog.pm. There are good reasons as suggested upthread
> >> to apply higher standards to library files than to, say, a TAP test
> >> script. The only easy way I can see to do that would be to have two
> >> different perlcriticrc files and adjust pgperlcritic to make two runs.
> >> If people think that's worth it I'll put a little work into it. If not,
> >> I'll just leave things here.
> > I think being more strict about it in strategic files (I'd say that's
> > Catalog.pm plus src/test/perl/*.pm) might be a good idea. Maybe give it
> > a try and see what comes up.
>
> OK, in fact those files are in reasonably good shape. I also took a pass
> through the library files in src/tools/msvc, which had a few more issues.

It would be an unpleasant surprise to cause a perlcritic buildfarm failure by
moving a function, verbatim, from a non-strategic file to a strategic file.
Having two Perl style regimes in one tree is itself a liability.

> --- a/src/backend/catalog/Catalog.pm
> +++ b/src/backend/catalog/Catalog.pm
> @@ -67,7 +67,7 @@ sub ParseHeader
> if (!$is_client_code)
> {
> # Strip C-style comments.
> - s;/\*(.|\n)*\*/;;g;
> + s;/\*(?:.|\n)*\*/;;g;

This policy against unreferenced groups makes the code harder to read, and the
chance of preventing a bug is too low to justify that.

> --- a/src/tools/perlcheck/pgperlcritic
> +++ b/src/tools/perlcheck/pgperlcritic
> @@ -14,7 +14,21 @@ PERLCRITIC=${PERLCRITIC:-perlcritic}
>
> . src/tools/perlcheck/find_perl_files
>
> -find_perl_files | xargs $PERLCRITIC \
> +flist=`mktemp`
> +find_perl_files > $flist
> +
> +pattern='src/test/perl/|src/backend/catalog/Catalog.pm|src/tools/msvc/[^/]*.pm'

I don't find these files to be especially strategic, and I'm mostly shrugging
about the stricter policy's effect on code quality. -1 for this patch.