From: Noah Misch <noah@leadboat.com>
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: cleaning perl code
Date: Sat, 11 Apr 2020 04:30:14 +0000
Message-ID: <20200411043014.GA590991@rfd.leadboat.com> (raw)
In-Reply-To: <e56f6fb6-38d5-8450-577c-24eb5d63f2e1@2ndQuadrant.com>
References: <e56f6fb6-38d5-8450-577c-24eb5d63f2e1@2ndQuadrant.com>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com, andrew.dunstan@2ndquadrant.com
Subject: Re: cleaning perl code
In-Reply-To: <20200411043014.GA590991@rfd.leadboat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox