Обсуждение: Cleaning up perl code
Hello hackers, Please look at a bunch of unused variables and a couple of other defects I found in the perl code, maybe you'll find them worth fixing: contrib/amcheck/t/001_verify_heapam.pl $result # unused since introduction in 866e24d47 unused sub: get_toast_for # not used since 860593ec3 contrib/amcheck/t/002_cic.pl $result # orphaned since 7f580aa5d src/backend/utils/activity/generate-wait_event_types.pl $note, $note_name # unused since introduction in fa8892847 src/bin/pg_dump/t/003_pg_dump_with_server.pl $cmd, $stdout, $stderr, $result # unused since introduction in 2f9eb3132 src/bin/pg_dump/t/005_pg_dump_filterfile.pl $cmd, $stdout, $stderr, $result # unused since introduciton in a5cf808be src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl $slapd, $ldap_schema_dir # unused since introduction in 419a8dd81 src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl $clearpass # orphaned since b846091fd src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm $ostmt # unused since introduction in 47bb9db75 src/test/recovery/t/021_row_visibility.pl $ret # unused since introduction in 7b28913bc src/test/recovery/t/032_relfilenode_reuse.pl $ret # unused since introduction in e2f65f425 src/test/recovery/t/035_standby_logical_decoding.pl $stdin, $ret, $slot # unused since introduction in fcd77d532 $subscriber_stdin, $subscriber_stdout, $subscriber_stderr # unused since introduction in 376dc8205 src/test/subscription/t/024_add_drop_pub.pl invalid reference in a comment: tab_drop_refresh -> tab_2 # introduced with 1046a69b3 (invalid since v6-0001-...patch in the commit's thread) src/tools/msvc_gendef.pl @def # unused since introduction in 933b46644? I've attached a patch with all of these changes (tested with meson build on Windows and check-world on Linux). Best regards, Alexander
Вложения
Alexander Lakhin <exclusion@gmail.com> writes: > Hello hackers, > > Please look at a bunch of unused variables and a couple of other defects > I found in the perl code, maybe you'll find them worth fixing: Nice cleanup! Did you use some static analysis tool, or did look for them manually? If I add [Variables::ProhibitUnusedVariables] to src/tools/perlcheck/perlcriticrc, it finds a few more, see the attached patch. The scripts parsing errcodes.txt really should be refactored into using a common module, but that's a patch for another day. - ilmari From 6b096a39753338bb91add5fcf1ed963024e58c15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org> Date: Mon, 20 May 2024 19:55:20 +0100 Subject: [PATCH] Prohibit unused variables --- src/pl/plpgsql/src/generate-plerrcodes.pl | 6 ++---- src/pl/plpython/generate-spiexceptions.pl | 6 ++---- src/pl/tcl/generate-pltclerrcodes.pl | 6 ++---- src/tools/perlcheck/perlcriticrc | 2 ++ src/tools/pgindent/pgindent | 2 +- 5 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/pl/plpgsql/src/generate-plerrcodes.pl b/src/pl/plpgsql/src/generate-plerrcodes.pl index 1c662bc967..e969a4b33e 100644 --- a/src/pl/plpgsql/src/generate-plerrcodes.pl +++ b/src/pl/plpgsql/src/generate-plerrcodes.pl @@ -23,10 +23,8 @@ # Skip section headers next if /^Section:/; - die unless /^([^\s]{5})\s+([EWS])\s+([^\s]+)(?:\s+)?([^\s]+)?/; - - (my $sqlstate, my $type, my $errcode_macro, my $condition_name) = - ($1, $2, $3, $4); + my ($type, $errcode_macro, $condition_name) = + /^[^\s]{5}\s+([EWS])\s+([^\s]+)(?:\s+)?([^\s]+)?/ or die; # Skip non-errors next unless $type eq 'E'; diff --git a/src/pl/plpython/generate-spiexceptions.pl b/src/pl/plpython/generate-spiexceptions.pl index f0c5142be3..984017f212 100644 --- a/src/pl/plpython/generate-spiexceptions.pl +++ b/src/pl/plpython/generate-spiexceptions.pl @@ -23,10 +23,8 @@ # Skip section headers next if /^Section:/; - die unless /^([^\s]{5})\s+([EWS])\s+([^\s]+)(?:\s+)?([^\s]+)?/; - - (my $sqlstate, my $type, my $errcode_macro, my $condition_name) = - ($1, $2, $3, $4); + my ($type, $errcode_macro, $condition_name) = + /^[^\s]{5}\s+([EWS])\s+([^\s]+)(?:\s+)?([^\s]+)?/ or die; # Skip non-errors next unless $type eq 'E'; diff --git a/src/pl/tcl/generate-pltclerrcodes.pl b/src/pl/tcl/generate-pltclerrcodes.pl index fcac4d00a6..58eb6afefe 100644 --- a/src/pl/tcl/generate-pltclerrcodes.pl +++ b/src/pl/tcl/generate-pltclerrcodes.pl @@ -23,10 +23,8 @@ # Skip section headers next if /^Section:/; - die unless /^([^\s]{5})\s+([EWS])\s+([^\s]+)(?:\s+)?([^\s]+)?/; - - (my $sqlstate, my $type, my $errcode_macro, my $condition_name) = - ($1, $2, $3, $4); + my ($type, $errcode_macro, $condition_name) = + /^[^\s]{5}\s+([EWS])\s+([^\s]+)(?:\s+)?([^\s]+)?/ or die; # Skip non-errors next unless $type eq 'E'; diff --git a/src/tools/perlcheck/perlcriticrc b/src/tools/perlcheck/perlcriticrc index 4739e9f4f1..6053dfcc2a 100644 --- a/src/tools/perlcheck/perlcriticrc +++ b/src/tools/perlcheck/perlcriticrc @@ -15,6 +15,8 @@ verbose = %f: %m at line %l, column %c. %e. ([%p] Severity: %s)\n # Note: for policy descriptions see https://metacpan.org/dist/Perl-Critic +[Variables::ProhibitUnusedVariables] +severity = 5 # allow octal constants with leading zeros [-ValuesAndExpressions::ProhibitLeadingZeros] diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index 48d83bc434..063ec8ce63 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -22,7 +22,7 @@ my $indent_opts = my $devnull = File::Spec->devnull; my ($typedefs_file, $typedef_str, @excludes, - $indent, $build, $diff, + $indent, $diff, $check, $help, @commits,); $help = 0; -- 2.39.2
Hello Dagfinn, Thank you for paying attention to it and improving the possible fix! 20.05.2024 23:39, Dagfinn Ilmari Mannsåker wrote: > Nice cleanup! Did you use some static analysis tool, or did look for > them manually? I reviewed my collection of unica I gathered for several months, but had found some of them too minor/requiring more analysis. Then I added more with perlcritic's policy UnusedVariables, and also checked for unused subs with a script from blogs.perl.org (and it confirmed my only previous find of that kind). > If I add [Variables::ProhibitUnusedVariables] to > src/tools/perlcheck/perlcriticrc, it finds a few more, see the attached > patch. Yes, I saw unused $sqlstates, but decided that they are meaningful enough to stay. Though maybe enabling ProhibitUnusedVariables justifies fixing them too. > The scripts parsing errcodes.txt really should be refactored into using > a common module, but that's a patch for another day. Agree, and I would leave 005_negotiate_encryption.pl (with $node_conf, $server_config unused since d39a49c1e) aside for another day too. Best regards, Alexander
On Tue, May 21, 2024 at 06:00:00AM +0300, Alexander Lakhin wrote: > I reviewed my collection of unica I gathered for several months, but had > found some of them too minor/requiring more analysis. > Then I added more with perlcritic's policy UnusedVariables, and also > checked for unused subs with a script from blogs.perl.org (and it confirmed > my only previous find of that kind). Nice catches from both of you. The two ones in generate-wait_event_types.pl are caused by me, actually. Not sure about the changes in the errcodes scripts, though. The current state of thing can be also useful when it comes to debugging the parsing, and it does not hurt to keep the parsing rules the same across the board. >> The scripts parsing errcodes.txt really should be refactored into using >> a common module, but that's a patch for another day. > > Agree, and I would leave 005_negotiate_encryption.pl (with $node_conf, > $server_config unused since d39a49c1e) aside for another day too. I'm not sure about these ones as each one of these scripts have their own local tweaks. Now, if there is a cleaner picture with a .pm module I don't see while reading the whole, why not as long as it improves the code. -- Michael
Вложения
On Tue, May 21, 2024 at 02:33:16PM +0900, Michael Paquier wrote: > Nice catches from both of you. The two ones in > generate-wait_event_types.pl are caused by me, actually. > > Not sure about the changes in the errcodes scripts, though. The > current state of thing can be also useful when it comes to debugging > the parsing, and it does not hurt to keep the parsing rules the same > across the board. For now, I have staged for commit the attached, that handles most of the changes from Alexander (msvc could go for more cleanup?). I'll look at the changes from Dagfinn after that, including if perlcritic could be changed. I'll handle the first part when v18 opens up, as that's cosmetic. The incorrect comment in 024_add_drop_pub.pl has been fixed as of 53785d2a2aaa, as that was a separate issue. -- Michael