Re: cleaning perl code - Mailing list pgsql-hackers

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