Date: Thu, 16 Apr 2020 11:12:48 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: "Hamlin, Garick L" <ghamlin@isc.upenn.edu>
Cc: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>,
	Noah Misch <noah@leadboat.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: cleaning perl code
Message-ID: <20200416151248.GA8282@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200416142052.zqtgtiubaf6fycn2@isc.upenn.edu>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

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).

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).

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.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services