On 4/12/20 3:22 PM, Robert Haas wrote:
On Sat, Apr 11, 2020 at 11:15 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
Noah Misch <n...@leadboat.com> writes:
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.
If it's possible to turn off just that warning, then +several.
It's routinely caused buildfarm failures, yet I can detect exactly
no value in it. If there were sufficient cross-procedural analysis
backing it to detect whether any caller examines the subroutine's
result value, then it'd be worth having. But there isn't, so those
extra returns are just pedantic verbosity.
I agree with Noah's comment about CPAN: it would be worth being more
careful about things like this if we were writing code that was likely
to be used by a wide variety of people and a lot of code over which we
have no control and which we do not get to even see. But that's not
the case here. It does not seem worth stressing the authors of TAP
tests over such things.
FWIW, pgBackRest used Perl Critic when we were distributing Perl code
but stopped when our Perl code was only used for integration testing.
Perhaps that was the wrong call but we decided the extra time required
to run it was not worth the benefit. Most new test code is written in C
and the Perl test code is primarily in maintenance mode now.
When we did use Perl Critic we set it at level 1 (--brutal) and then
wrote an exception file for the stuff we wanted to ignore. The advantage
of this is that if new code violated a policy that did not already have
an exception we could evaluate it and either add an exception or modify
the code. In practice this was pretty rare, but we also had a short
excuse for many exceptions and a list of exceptions that should be
re-evaluated in the future.
About the time we introduced Perl Critic we were already considering the
C migration so most of the exceptions stayed.
Just in case it is useful, I have attached our old policy file with
exceptions and excuses (when we had one).
Regards,
--
-David
da...@pgmasters.net
# Main Perl Critic Policy Applied to Entire Code Base
#-----------------------------------------------------------------------------------------------------------------------------------
# Important policies that should always be checked
#-----------------------------------------------------------------------------------------------------------------------------------
[TestingAndDebugging::RequireUseStrict]
severity = 5
[TestingAndDebugging::RequireUseWarnings]
severity = 5
# Permanent Exceptions
#-----------------------------------------------------------------------------------------------------------------------------------
# Requires all local variables to be all lower/upper case -- can't see how this
is a good thing.
[-NamingConventions::Capitalization]
# Requires @_ to be immediately unpacked but won't work with param logging
scheme.
[-Subroutines::RequireArgUnpacking]
# Requires all exports to be configurable by caller. This is fine for
independent libraries but
# overly burdensome for modules integrated as part of an application. Maybe
apply to certain modules
# that are also used by doc and test programs?
[-Modules::ProhibitAutomaticExportation]
# Requires built-in functions to not have parens, but in this project it is
preferred to wrap all
# function calls in parens for consistency.
[-CodeLayout::ProhibitParensWithBuiltins]
# Requires module version vars. Probably not practical for built-in modules.
[-Modules::RequireVersionVar]
# Requires extended formatting for all regexps. Seems overly burdensome, or at
least something to look
# at a lot further down the road.
[-RegularExpressions::RequireExtendedFormatting]
# Requires Unicode safe expressions. May be worth looking at sometime.
[-RegularExpressions::ProhibitEnumeratedClasses]
# S2 - Requires List::MoreUtils instead of boolean grep. Not worth it to load
another module.
[-BuiltinFunctions::ProhibitBooleanGrep]
# Provisional Exceptions for Test & Documentation Code
#-----------------------------------------------------------------------------------------------------------------------------------
# S2 - Requires complete POD sections but these are being removed anyway in
favor of Config.pm.
[-Documentation::RequirePodSections]
[-Documentation::RequirePodAtEnd]
# S3 - Requires regexps to be below a certain length. Seems burdensome.
[-RegularExpressions::ProhibitComplexRegexes]
# To Be Fixed or Evaluated
#
# Natural ordering here indicates the order in which they should be addressed.
#-----------------------------------------------------------------------------------------------------------------------------------
# S2 - Requires all long numbers to have thousand separators. Probably a good
idea bit need to change a fair amount of code.
[-ValuesAndExpressions::RequireNumberSeparators]
# S4 - Requires parens when logical and bitwise booleans are mixed.
[-ValuesAndExpressions::ProhibitMixedBooleanOperators]
# S4 - Requires that sub names not overlap with built-ins - a bummer for object
members.
[-Subroutines::ProhibitBuiltinHomonyms]
# S4 - Requires block form of grep for readability. Needs to be fixed in about
15 places.
[-BuiltinFunctions::RequireBlockGrep]
# S4 - Requires modification of certain vars (e.g. $SIG) to have local scope.
Needs to be fixed in about 20 places.
[-Variables::RequireLocalizedPunctuationVars]
# S4 - Requires close() to be called soon after open but seems arbitrary.
[-InputOutput::RequireBriefOpen]
# S1 - Requires reverse keyword for reverse sorts instead of block. May not be
able to since $a $b are passed as a parameter.
[-BuiltinFunctions::ProhibitReverseSortBlock]
# S3 - Requires use of Carp instead of die or warn. Doesn't seem useful.
[-ErrorHandling::RequireCarping]
# S3 - Requires use of local vars in packages. Can't use as it prohibits use of
$DBI::errstr.
[-Variables::ProhibitPackageVars]
# S3 - Requires that certain operators not be mixed.
[-ValuesAndExpressions::ProhibitMismatchedOperators]
# S2 - Requires use of if instead of unless.
[-ControlStructures::ProhibitUnlessBlocks]
# S1 - Requires true literals to use single quotes.
[-ValuesAndExpressions::ProhibitInterpolationOfLiterals]
# S2 - Requires split expressions to be regexp for clarity.
[-BuiltinFunctions::ProhibitStringySplit]
# S4 - Requires use of Readonly instead of const. Has performance and syntax
advantages.
[-ValuesAndExpressions::ProhibitConstantPragma]
# S2 - Requires all numbers to be defined as constants
[-ValuesAndExpressions::ProhibitMagicNumbers]
# S4 - Requires all subs to have a return, even if there is not value to return.
[-Subroutines::RequireFinalReturn]
# S4 - Requires new to be called as Object->new().
[-Objects::ProhibitIndirectSyntax]
# S2 - Requires that & not be used in functions calls. Currently this is used
a lot for &log() calls.
[-Subroutines::ProhibitAmpersandSigils]
# S2 - Requires use of eq instead of regexp when possible.
[-RegularExpressions::ProhibitFixedStringMatches]
# S2 - Requires that sigils be separated by braces, eg %$var becomes %{$var}.
[-References::ProhibitDoubleSigils]
# S2 - Requires use English instead a puctuation vars such as $!.
[-Variables::ProhibitPunctuationVars]
# S3 - Requires nested if/else have limited depth and recommends using
given/when instead.
[-ControlStructures::ProhibitCascadingIfElse]
# S2 - Requires empty strings to be represented with qw{}.
[-ValuesAndExpressions::ProhibitEmptyQuotes]
# S2 - Requires non letter and number strings to be represented with something
like qw{/}.
[-ValuesAndExpressions::ProhibitNoisyQuotes]
# S2 - Requires expanded matching for . in regular expressions.
[-RegularExpressions::RequireDotMatchAnything]
# S2 - Requires sed-style boundary matching. May not be appropriate for reg
exps in this project, though.
[-RegularExpressions::RequireLineBoundaryMatching]
# S1 - Requires use of Perl::Tidy.
[-CodeLayout::RequireTidyCode]
# S2 - Requires use of Perl syntax for simple loops, e.g. for (0..$max).
[-ControlStructures::ProhibitCStyleForLoops]
# S2 - Require standard if structures rather than postfix for readability.
[-ControlStructures::ProhibitPostfixControls]
# S3 - Requires code have a McCabe score of no more than 20 but is configurable.
[-Subroutines::ProhibitExcessComplexity]
[-Modules::ProhibitExcessMainComplexity]
# S3 - Requires low level of code nesting (may require a lot of refactoring).
[-ControlStructures::ProhibitDeepNests]
# S3 - Requires subs to have <= 6 args but is configurable.
[-Subroutines::ProhibitManyArgs]
# S3 - Requires arbitrary unambigious names but is configurable.
[-NamingConventions::ProhibitAmbiguousNames]
# S3 - Requires that var names never be resused in a sub, not sure about this
one.
[-Variables::ProhibitReusedNames]
# S3 - Requires non-capturing groups in regexp, primarily a performance
optimization.
[-RegularExpressions::ProhibitUnusedCapture]
# S1 - Requires trailing commas on all lists.
[-CodeLayout::RequireTrailingCommas]
# S2 - Requires chained calls be less than four.
[-ValuesAndExpressions::ProhibitLongChainsOfMethodCalls]
# S2 - Requires check for success of close() function.
[-InputOutput::RequireCheckedClose]
# S1 - Requires use Fatal or autodie with syscalls.
[-InputOutput::RequireCheckedSyscalls]
# S1 - Requires less abiguity for metacharacters in strings.
[-ValuesAndExpressions::RequireInterpolationOfMetachars]
# S4 - Requires character classes to reduce escapes in regexps.
[-RegularExpressions::ProhibitEscapedMetacharacters]
# S1 - Require character classes rather than single character alternation.
[-RegularExpressions::ProhibitSingleCharAlternation]
# S2 - Require qw{} syntax for quoted string lists
[-CodeLayout::ProhibitQuotedWordLists]
