Re: cleaning perl code - Mailing list pgsql-hackers

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.


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:

$ find src -name '*.p[lm]'| xargs grep -n '^.return;' | wc -l
194