Thread: 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
Attachment
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
Attachment
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
Attachment
On Fri, May 24, 2024 at 02:09:49PM +0900, Michael Paquier wrote: > For now, I have staged for commit the attached, that handles most of > the changes from Alexander (msvc could go for more cleanup?). This one has been applied as of 0c1aca461481 now that v18 is open. > 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. I'm still biased about the second set of changes proposed here, though. ProhibitUnusedVariables would have benefits when writing perl code in terms of clarity because we would avoid useless stuff, but it seems to me that we should put more efforts into the unification of the errcodes parsing paths first to have a cleaner long-term picture. That's not directly the fault of this proposal that we have the same parsing rules spread across three PL languages, so perhaps what's proposed is fine as-is, at the end. Any thoughts or comments from others more familiar with ProhibitUnusedVariables? -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes: > On Fri, May 24, 2024 at 02:09:49PM +0900, Michael Paquier wrote: >> For now, I have staged for commit the attached, that handles most of >> the changes from Alexander (msvc could go for more cleanup?). > > This one has been applied as of 0c1aca461481 now that v18 is > open. > >> 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. For clarity, I've rebased my addional unused-variable changes (except the errcodes-related ones, see below) onto current master, and split it into separate commits with detailed explaiations for each file file, see attached. > I'm still biased about the second set of changes proposed here, > though. ProhibitUnusedVariables would have benefits when writing perl > code in terms of clarity because we would avoid useless stuff, but it > seems to me that we should put more efforts into the unification of > the errcodes parsing paths first to have a cleaner long-term picture. > > That's not directly the fault of this proposal that we have the same > parsing rules spread across three PL languages, so perhaps what's > proposed is fine as-is, at the end. It turns out there are a couple more places that parse errcodes.txt, namely doc/src/sgml/generate-errcodes-table.pl and src/backend/utils/generate-errcodes.pl. I'll have a go refactoring all of these into a common function à la Catalog::ParseHeader() that returns a data structure these scripts can use as as appropriate. > Any thoughts or comments from others more familiar with > ProhibitUnusedVariables? Relatedly, I also had a look at prohibiting unused regex captures (RegularExpressions::ProhibitUnusedCapture), which found a few real cases, but also lots of false positives in Catalog.pm, because it doesn't understand that %+ uses all named captures, so I won't propose a patch for that until that's fixed upstream (https://github.com/Perl-Critic/Perl-Critic/pull/1065). - ilmari From 8e3c7721da86a670b279bb24b0fe9ae82351628f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org> Date: Tue, 2 Jul 2024 13:25:16 +0100 Subject: [PATCH 1/3] Remove unused variables in libq encryption negotiation test The $node_conf parameter to test_matrix() was only ever used to set the %params hash (removed in commit d39a49c1e4598048), but that hash was itself never used for anything, so it (and the $server_config) hash were always useless. --- .../libpq/t/005_negotiate_encryption.pl | 24 +++++-------------- 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/src/interfaces/libpq/t/005_negotiate_encryption.pl b/src/interfaces/libpq/t/005_negotiate_encryption.pl index c3f70d31bc..251c405926 100644 --- a/src/interfaces/libpq/t/005_negotiate_encryption.pl +++ b/src/interfaces/libpq/t/005_negotiate_encryption.pl @@ -215,11 +215,6 @@ BEGIN my @all_sslmodes = ('disable', 'allow', 'prefer', 'require'); my @all_sslnegotiations = ('postgres', 'direct'); -my $server_config = { - server_ssl => 0, - server_gss => 0, -}; - ### ### Run tests with GSS and SSL disabled in the server ### @@ -272,7 +267,7 @@ BEGIN }; note("Running tests with SSL and GSS disabled in the server"); -test_matrix($node, $server_config, +test_matrix($node, ['testuser'], \@all_gssencmodes, \@all_sslmodes, \@all_sslnegotiations, parse_table($test_table)); @@ -311,19 +306,15 @@ BEGIN # Enable SSL in the server $node->adjust_conf('postgresql.conf', 'ssl', 'on'); $node->reload; - $server_config->{server_ssl} = 1; note("Running tests with SSL enabled in server"); - test_matrix( - $node, $server_config, - [ 'testuser', 'ssluser', 'nossluser' ], ['disable'], - \@all_sslmodes, \@all_sslnegotiations, + test_matrix($node, [ 'testuser', 'ssluser', 'nossluser' ], + ['disable'], \@all_sslmodes, \@all_sslnegotiations, parse_table($test_table)); # Disable SSL again $node->adjust_conf('postgresql.conf', 'ssl', 'off'); $node->reload; - $server_config->{server_ssl} = 0; } ### @@ -336,7 +327,6 @@ BEGIN $krb->create_principal('gssuser', $gssuser_password); $krb->create_ticket('gssuser', $gssuser_password); - $server_config->{server_gss} = 1; $test_table = q{ # USER GSSENCMODE SSLMODE SSLNEGOTIATION EVENTS -> OUTCOME @@ -400,7 +390,7 @@ BEGIN } note("Running tests with GSS enabled in server"); - test_matrix($node, $server_config, [ 'testuser', 'gssuser', 'nogssuser' ], + test_matrix($node, [ 'testuser', 'gssuser', 'nogssuser' ], \@all_gssencmodes, $sslmodes, $sslnegotiations, parse_table($test_table)); } @@ -423,7 +413,6 @@ BEGIN # Enable SSL $node->adjust_conf('postgresql.conf', 'ssl', 'on'); $node->reload; - $server_config->{server_ssl} = 1; $test_table = q{ # USER GSSENCMODE SSLMODE SSLNEGOTIATION EVENTS -> OUTCOME @@ -510,7 +499,6 @@ BEGIN note("Running tests with both GSS and SSL enabled in server"); test_matrix( $node, - $server_config, [ 'testuser', 'gssuser', 'ssluser', 'nogssuser', 'nossluser' ], \@all_gssencmodes, \@all_sslmodes, @@ -546,8 +534,8 @@ sub test_matrix { local $Test::Builder::Level = $Test::Builder::Level + 1; - my ($pg_node, $node_conf, - $test_users, $gssencmodes, $sslmodes, $sslnegotiations, %expected) + my ($pg_node, $test_users, $gssencmodes, $sslmodes, $sslnegotiations, + %expected) = @_; foreach my $test_user (@{$test_users}) -- 2.39.2 From 7127c5a8a45bdb1549fbd99cd37523fc1f848169 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org> Date: Tue, 2 Jul 2024 13:35:48 +0100 Subject: [PATCH 2/3] msvc_gendef.pl: Remove unused variable Commit a5eed4d770674904 swithced to using a hash instead of an array for the defs, but neglected to remove the unused variable. --- src/tools/msvc_gendef.pl | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/tools/msvc_gendef.pl b/src/tools/msvc_gendef.pl index 404076dbbc..97346cc892 100644 --- a/src/tools/msvc_gendef.pl +++ b/src/tools/msvc_gendef.pl @@ -6,8 +6,6 @@ use List::Util qw(min); use Getopt::Long; -my @def; - # # Script that generates a .DEF file for all objects in a directory # -- 2.39.2 From ef4aed0fb3fe7d96662b0b9a5c6b5998916d9d4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org> Date: Tue, 2 Jul 2024 13:37:54 +0100 Subject: [PATCH 3/3] pgindent: remove unused variable Commit b16259b3c1897cf9 removed the --build option, but neglected to remove the corresponding $build variable. --- src/tools/pgindent/pgindent | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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
On 2024-07-02 Tu 8:55 AM, Dagfinn Ilmari Mannsåker wrote:
Relatedly, I also had a look at prohibiting unused regex captures (RegularExpressions::ProhibitUnusedCapture), which found a few real cases, but also lots of false positives in Catalog.pm, because it doesn't understand that %+ uses all named captures, so I won't propose a patch for that until that's fixed upstream (https://github.com/Perl-Critic/Perl-Critic/pull/1065).
We could mark Catalog.pm with a "## no critic (ProhibitUnusedCapture)" and then use the test elsewhere.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes: > On 2024-07-02 Tu 8:55 AM, Dagfinn Ilmari Mannsåker wrote: >> Relatedly, I also had a look at prohibiting unused regex captures >> (RegularExpressions::ProhibitUnusedCapture), which found a few real >> cases, but also lots of false positives in Catalog.pm, because it >> doesn't understand that %+ uses all named captures, so I won't propose a >> patch for that until that's fixed upstream >> (https://github.com/Perl-Critic/Perl-Critic/pull/1065). >> > > We could mark Catalog.pm with a "## no critic (ProhibitUnusedCapture)" > and then use the test elsewhere. Yeah, that's what I've done for now. Here's a sequence of patches that fixes the existing cases of unused captures, and adds the policy and override. The seg-validate.pl script seems unused, unmaintained and useless (it doesn't actually match the syntax accepted by seg, specifcially the (+-) syntax (which my patch fixes in passing)), so maybe we should just delete it instead? > cheers > > andrew -ilmari From 17a350c8d6ec2ae887fe7784dd6b9275028dc681 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org> Date: Mon, 20 May 2024 22:54:52 +0100 Subject: [PATCH 1/3] seg-validate.pl: Use qr// instead of strings to assemble regexes Also eliminate unused captures, and fix the plus/minus syntax to accept (+-), not the erroneous '+-'. --- contrib/seg/seg-validate.pl | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/contrib/seg/seg-validate.pl b/contrib/seg/seg-validate.pl index 22cbca966d..518fbfb24a 100755 --- a/contrib/seg/seg-validate.pl +++ b/contrib/seg/seg-validate.pl @@ -5,15 +5,15 @@ use strict; use warnings FATAL => 'all'; -my $integer = '[+-]?[0-9]+'; -my $real = '[+-]?[0-9]+\.[0-9]+'; +my $integer = qr/[+-]?[0-9]+/; +my $real = qr/[+-]?[0-9]+\.[0-9]+/; -my $RANGE = '(\.\.)(\.)?'; -my $PLUMIN = q(\'\+\-\'); -my $FLOAT = "(($integer)|($real))([eE]($integer))?"; -my $EXTENSION = '<|>|~'; +my $RANGE = qr/\.{2,3}/; +my $PLUMIN = qr/\Q(+-)/; +my $FLOAT = qr/(?:$integer|$real)(?:[eE]$integer)?/; +my $EXTENSION = qr/[<>~]/; -my $boundary = "($EXTENSION)?$FLOAT"; +my $boundary = qr/$EXTENSION?$FLOAT/; my $deviation = $FLOAT; my $rule_1 = $boundary . $PLUMIN . $deviation; @@ -28,23 +28,23 @@ { # s/ +//g; - if (/^($rule_1)$/) + if (/^$rule_1$/) { print; } - elsif (/^($rule_2)$/) + elsif (/^$rule_2$/) { print; } - elsif (/^($rule_3)$/) + elsif (/^$rule_3$/) { print; } - elsif (/^($rule_4)$/) + elsif (/^$rule_4$/) { print; } - elsif (/^($rule_5)$/) + elsif (/^$rule_5$/) { print; } -- 2.39.2 From 31ab31f409ea3305105822d91fd688ecd35b594d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org> Date: Tue, 2 Jul 2024 16:25:15 +0100 Subject: [PATCH 2/3] Fix unused regular expression captures Remove unused captures, actually use the capture, or fix the capture number used, as appropriate. --- src/backend/catalog/Catalog.pm | 2 +- src/backend/utils/activity/generate-wait_event_types.pl | 5 ++--- src/backend/utils/mb/Unicode/convutils.pm | 4 ++-- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm index 8e709524cb..59ebe5f434 100644 --- a/src/backend/catalog/Catalog.pm +++ b/src/backend/catalog/Catalog.pm @@ -69,7 +69,7 @@ sub ParseHeader if (!$is_client_code) { # Strip C-style comments. - s;/\*(.|\n)*\*/;;g; + s;/\*.*\*/;;gs; if (m;/\*;) { diff --git a/src/backend/utils/activity/generate-wait_event_types.pl b/src/backend/utils/activity/generate-wait_event_types.pl index 2f2fa5bb8f..4e8dc63e6b 100644 --- a/src/backend/utils/activity/generate-wait_event_types.pl +++ b/src/backend/utils/activity/generate-wait_event_types.pl @@ -55,10 +55,9 @@ next if /^\s*$/; # Get waitclassname based on the section - if (/^Section: ClassName(.*)/) + if (/^Section: ClassName - (\w+)/) { - $section_name = $_; - $section_name =~ s/^.*- //; + $section_name = $1; $abi_compatibility = 0; next; } diff --git a/src/backend/utils/mb/Unicode/convutils.pm b/src/backend/utils/mb/Unicode/convutils.pm index ee780acbe3..1b453d1eba 100644 --- a/src/backend/utils/mb/Unicode/convutils.pm +++ b/src/backend/utils/mb/Unicode/convutils.pm @@ -41,7 +41,7 @@ sub read_source next if (/^$/); # Ignore empty lines - next if (/^0x([0-9A-F]+)\s+(#.*)$/); + next if (/^0x[0-9A-F]+\s+#.*$/); # The Unicode source files have three columns # 1: The "foreign" code (in hex) @@ -55,7 +55,7 @@ sub read_source my $out = { code => hex($1), ucs => hex($2), - comment => $4, + comment => $3, direction => BOTH, f => $fname, l => $. -- 2.39.2 From 3558190b163378d810b5af405992ebd475c1f8a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org> Date: Tue, 2 Jul 2024 16:26:10 +0100 Subject: [PATCH 3/3] Prohibit unused regular expression captures Disable the policy in Catalog::ParseHeader, since perlcritic doesn't (yet) realise that %+ constitutes using all named captures. --- src/backend/catalog/Catalog.pm | 5 +++++ src/tools/perlcheck/perlcriticrc | 3 +++ 2 files changed, 8 insertions(+) diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm index 59ebe5f434..8f62ad6153 100644 --- a/src/backend/catalog/Catalog.pm +++ b/src/backend/catalog/Catalog.pm @@ -91,6 +91,10 @@ sub ParseHeader # Push the data into the appropriate data structure. # Caution: when adding new recognized OID-defining macros, # also update src/include/catalog/renumber_oids.pl. + # + # perlcritic doesn't know that %+ uses all named captures + # (https://github.com/Perl-Critic/Perl-Critic/pull/1065) + ## no critic (ProhibitUnusedCapture) if (/^DECLARE_TOAST\(\s* (?<parent_table>\w+),\s* (?<toast_oid>\d+),\s* @@ -194,6 +198,7 @@ sub ParseHeader $catalog{schema_macro} = /BKI_SCHEMA_MACRO/ ? 1 : 0; $declaring_attributes = 1; } + ## use critic elsif ($is_client_code) { if (/^#endif/) diff --git a/src/tools/perlcheck/perlcriticrc b/src/tools/perlcheck/perlcriticrc index 6053dfcc2a..ba238b2baa 100644 --- a/src/tools/perlcheck/perlcriticrc +++ b/src/tools/perlcheck/perlcriticrc @@ -18,6 +18,9 @@ verbose = %f: %m at line %l, column %c. %e. ([%p] Severity: %s)\n [Variables::ProhibitUnusedVariables] severity = 5 +[RegularExpressions::ProhibitUnusedCapture] +severity = 5 + # allow octal constants with leading zeros [-ValuesAndExpressions::ProhibitLeadingZeros] -- 2.39.2
On Tue, Jul 02, 2024 at 01:55:25PM +0100, Dagfinn Ilmari Mannsåker wrote: > For clarity, I've rebased my addional unused-variable changes (except > the errcodes-related ones, see below) onto current master, and split it > into separate commits with detailed explaiations for each file file, see > attached. Thanks, I've squashed these three ones into a single commit for now (good catches for 005_negotiate_encryption.pl, btw), and applied them. -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes: > On Tue, Jul 02, 2024 at 01:55:25PM +0100, Dagfinn Ilmari Mannsåker wrote: >> For clarity, I've rebased my addional unused-variable changes (except >> the errcodes-related ones, see below) onto current master, and split it >> into separate commits with detailed explaiations for each file file, see >> attached. > > Thanks, I've squashed these three ones into a single commit for now > (good catches for 005_negotiate_encryption.pl, btw), and applied them. Thanks! - ilmari