From: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Subject: Re: cleaning perl code
To: Noah Misch <noah@leadboat.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
References: <20200414204432.GA22922@alvherre.pgsql>
 <86b0e20d-efc1-4169-aeb1-be49b3a375ab@2ndQuadrant.com>
 <20200416030101.GB977691@rfd.leadboat.com>
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: <ee6dff48-485d-974d-cc68-67ee86fe73b8@2ndQuadrant.com>
Date: Thu, 16 Apr 2020 08:50:35 -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: <20200416030101.GB977691@rfd.leadboat.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


On 4/15/20 11:01 PM, Noah Misch wrote:
> 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.


Honestly, I think you're reaching here.


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


Non-capturing groups are also more efficient, and are something perl
programmers should be familiar with.


In fact, there's a much better renovation of semantics of this
particular instance, which is to make . match \n using the s modifier:


    s;/\*.*\*/;;gs;


It would also be more robust using non-greedy matching:


    s;/\*.*?\*/;;gs


After I wrote the above I went and looked at what we do the buildfarm
code to strip comments when looking for typedefs, and it's exactly that,
so at least I'm consistent :-)


I don't care that much if we throw this whole thing away. This was sent
in response to Alvaro's suggestion to "give it a try and see what comes up".


cheers


andrew


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