Re: Cleaning up perl code - Mailing list pgsql-hackers

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