Cleaning up perl code

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

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

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

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

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

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

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