From: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Hamlin, Garick L <ghamlin@isc.upenn.edu>
Cc: Noah Misch <noah@leadboat.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: cleaning perl code
Date: Thu, 16 Apr 2020 17:07:44 -0400
Message-ID: <e5f19183-7068-1a7d-4fec-df7ee22ef767@2ndQuadrant.com> (raw)
In-Reply-To: <20200416151248.GA8282@alvherre.pgsql>
References: <20200416151248.GA8282@alvherre.pgsql>
On 4/16/20 11:12 AM, Alvaro Herrera wrote:
> On 2020-Apr-16, Hamlin, Garick L wrote:
>
>> With the old expression 'something;' would be stripped away.
>> Is that an issue where this this is used? Why are we parsing
>> these headers?
> These are files from which bootstrap catalog data is generated, which is
> why we parse from Perl; but also where C structs are declared, which is
> why they're C.
>
> I think switching to non-greedy is a win in itself. Non-capturing
> parens is probably a wash (this doesn't run often so the performance
> argument isn't very interesting).
Yeah, I'm inclined to fix this independently of the perlcritic stuff.
The change is more readable and more correct as well as being perlcritic
friendly.
I might take a closer look at Catalog.pm.
Meanwhile, the other regex highlighted in the patch, in Solution.pm:
if (/^AC_INIT\(\[([^\]]+)\], \[([^\]]+)\], \[([^\]]+)\], \[([^\]]*)\],
\[([^\]]+)\]/)
is sufficiently horrid that I think we should see if we can rewrite it,
maybe as an extended regex. And a better fix here instead of marking the
fourth group as non-capturing would be simply to get rid of the parens
altogether. The serve no purpose at all.
>
> An example. This eval in Catalog.pm
>
> + ## no critic (ProhibitStringyEval)
> + ## no critic (RequireCheckingReturnValueOfEval)
> + eval '$hash_ref = ' . $_;
>
> is really weird stuff generally speaking, and the fact that we have to
> mark it specially for critic is a good indicator of that -- it serves as
> documentation. Catalog.pm is all a huge weird hack, but it's a critically
> important hack. Heck, what about RequireCheckingReturnValueOfEval --
> should we instead consider actually checking the return value of eval?
> It would seem to make sense, would it not? (Not for this patch, though
> -- I would be fine with just adding the nocritic line now, and removing
> it later while fixing that).
+1
>
> All in all, I think it's a positive value in having this code be checked
> with a bit more strength -- checks that are pointless in, say, t/00*.pl
> prove files.
thanks
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: andrew.dunstan@2ndquadrant.com, alvherre@2ndquadrant.com, ghamlin@isc.upenn.edu, noah@leadboat.com
Subject: Re: cleaning perl code
In-Reply-To: <e5f19183-7068-1a7d-4fec-df7ee22ef767@2ndQuadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox