From: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Subject: Re: cleaning perl code
To: Alvaro Herrera <alvherre@2ndquadrant.com>,
 "Hamlin, Garick L" <ghamlin@isc.upenn.edu>
Cc: Noah Misch <noah@leadboat.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
References: <20200416151248.GA8282@alvherre.pgsql>
Autocrypt: addr=andrew.dunstan@2ndQuadrant.com; prefer-encrypt=mutual;
 keydata=
 mQENBE7KWFkBCAClridxur2AIc7eW2AR7izbfp3EnNefie2HbLF0izW5Ik5UjX2HBXBx4syI
 gY6b0ugohXrr274+baoAlvSbq6cAoQuEVrk5IZFzt20b1Xkx65FwGSEj526yiKLocqkJceSq
 Xr9xcA5SGY+FZv441chh5SU92v4q6z+6LPpoHOh97ptAVXZYNTtU0LevyvD5lja0TzbvJm6C
 eFXitJfnm1pLEr0DGJCR/iUOl/N62Kh4855zZC7NHIjQHPOvV5Stz/l5ilDhvGVk+xkXFPys
 SjZoUr1rXhYLpiyi5sR0X9FHXT0KnGuz1F5ERO7ZTLSSQ6fJwPj6gOk9K+vvoKvoeql5ABEB
 AAG0L0FuZHJldyBEdW5zdGFuIDxhbmRyZXcuZHVuc3RhbkAybmRRdWFkcmFudC5jb20+iQFY
 BBMBAgBCAhsDBgsJCAcDAgYVCAIJCgsEFgIDAQIeAQIXgBYhBOQ+WEYd/Hy/RGkVpZn6f8tZ
 /DuBAhkBBQJd1pSYBQkSzqMsAAoJEJn6f8tZ/DuBJmEIAIVFBIC3qGqh+YTPVTkfowwxpHz0
 tZSPjZ6GS/5y62kgoZrZOVXw+Rg2ceuMM3a+In/RRjTx4bembgYqRKgzXlDfiNBJeq5ioVuN
 H1faDKE2lm5a6QkRQ2JU6DKf60fqSJGP5LisCUr1V0wz6EnlO31ArrKtXCNB+yk0KDAS6o6I
 Ql4pkXpr+vq4iilwJmiJOqTBam0LmQE7ulYCCYU8Z6MkpiSl+dO7QSramcGyxauQRI3a7gcq
 dpo8Vrv3gFTQDeMYxk7cqZp08pIVH5XR0+LgBz4X84f59rA/VAKlC/r6CyEQk59rvDLxCMyP
 2buoj51Z25N3MVghRDGcsU6mT7e5AQ0ETspYWQEIANGc4zQULOxhbqO2dyD51YhqCNRmm9oK
 Waqf+wmW4tpDe/VVcxAnNizd4LWCHfzpb5cHAtGkOPePMfzWVf6nvdF7d3eglbtf59+zG7O7
 llV0xSSoFiieQBsrGvqDInXYX/4mRRXMtyhM353/tixC9RWLs1oofyYmCPPXXY7h9R7en3B8
 BoVrRFcdzlIY/NFNhFGW/9dkEiGjgna2Rk6e15kln4ZvFBWUg23p93w/pqXcxY6+k/8TEk+C
 4R+M6w7o2PLGOjdZ+kPiUcw5H85zf/yZJwQXzisXaNduwWB6Vads9YC9dj6kPR1c4VGRqAaY
 L++LAEOqrlvm2TvqQqZRtnEAEQEAAYkBJQQYAQIADwIbDAUCXdaUtAUJEs6jVQAKCRCZ+n/L
 Wfw7gQi5B/9sHbmRloZNCWWwZGm+O9iG/JGQxS0WWa4GaZqzqSTWOft2/gO0Bgag89r1djq4
 PFgSZtzHvKQpqYz/7HkPrWh1jpcS83i/uYY1uyHXUUtpdEmn4buNObEtD0YNaJkGdhASsECq
 6akw+u7WJYmAHs/SpR6ok6HqLw7JHDVc2h5dViLfTk0qrgDa7sq/LVMoJje5Ct/Ku3YaWAV6
 YSmzoPP5BmPVFqP11OMQkCqurIu9vTLXKFWX623vCmqRDHJ6e5GCDHbH8USU+4wJBfPymZkd
 cA7NGm/mmWeJiWBFIDyjzYxrjku8ty9TZJVsaRXnS+OFQzXz9Eu2wTpKdHij46uY
Message-ID: <e5f19183-7068-1a7d-4fec-df7ee22ef767@2ndQuadrant.com>
Date: Thu, 16 Apr 2020 17:07:44 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <20200416151248.GA8282@alvherre.pgsql>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk


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