Re: cleaning perl code - Mailing list pgsql-hackers

On 4/11/20 12:30 AM, Noah Misch wrote:
> On Thu, Apr 09, 2020 at 11:44:11AM -0400, Andrew Dunstan wrote:
>>          39 Always unpack @_ first
> Requiring a "my @args = @_" does not improve this code:
>
> sub CreateSolution
> {
>     ...
>     if ($visualStudioVersion eq '12.00')
>     {
>         return new VS2013Solution(@_);
>     }
>
>>          30 Code before warnings are enabled
> Sounds good.  We already require "use strict" before code.  Requiring "use
> warnings" in the exact same place does not impose much burden.
>
>>          12 Subroutine "new" called using indirect syntax
> No, thanks.  "new VS2013Solution(@_)" and "VS2013Solution->new(@_)" are both
> fine; enforcing the latter is an ongoing waste of effort.
>
>>           9 Multiple "package" declarations
> This is good advice if you're writing for CPAN, but it would make PostgreSQL
> code worse by having us split affiliated code across multiple files.
>
>>           9 Expression form of "grep"
> No, thanks.  I'd be happier with the opposite, requiring grep(/x/, $arg)
> instead of grep { /x/ } $arg.  Neither is worth enforcing.
>
>>           7 Symbols are exported by default
> This is good advice if you're writing for CPAN.  For us, it just adds typing.
>
>>           5 Warnings disabled
>>           4 Magic variable "$/" should be assigned as "local"
>>           4 Comma used to separate statements
>>           2 Readline inside "for" loop
>>           2 Pragma "constant" used
>>           2 Mixed high and low-precedence booleans
>>           2 Don't turn off strict for large blocks of code
>>           1 Magic variable "@a" should be assigned as "local"
>>           1 Magic variable "$|" should be assigned as "local"
>>           1 Magic variable "$\" should be assigned as "local"
>>           1 Magic variable "$?" should be assigned as "local"
>>           1 Magic variable "$," should be assigned as "local"
>>           1 Magic variable "$"" should be assigned as "local"
>>           1 Expression form of "map"
> I looked less closely at the rest, but none give me a favorable impression.



I don't have a problem with some of this. OTOH, it's nice to know what
we're ignoring and what we're not.


What I have prepared is first a patch that lowers the severity level to
3 but implements policy exceptions so that nothing is broken. Then 3
patches. One fixes the missing warnings pragma and removes shebang -w
switches, so we are quite consistent about how we do this. I gather we
are agreed about that one. The next one fixes those magic variable
error. That includes using some more idiomatic perl, and in one case
just renaming a couple of variables that are fairly opaque anyway. The
last one fixes the mixture of high and low precedence boolean operators,
the inefficient <FOO> inside a foreach loop,  and the use of commas to
separate statements, and relaxes the policy about large blocks with 'no
strict'.


Since I have written them they are attached, for posterity if nothing
else. :-)



>
>
> In summary, among those warnings, I see non-negative value in "Code before
> warnings are enabled" only.  While we're changing this, I propose removing
> Subroutines::RequireFinalReturn.  Implicit return values were not a material
> source of PostgreSQL bugs, yet we've allowed this to litter our code:
>

That doesn't mean it won't be a source of problems in future, I've
actually been bitten by this in the past.


cheers


andrew


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