[PATCH] Clear up perlcritic 'missing return' warning
This is the first in a series of patches to reduce the number of warnings
from perlcritic. The current plan is to submit a separate patch for each
warning or small set of related warnings, to make reviewing more
straightforward.
This particular patch addresses the warning caused by falling off the end
of a subroutine rather than explicitly returning. It also contains
additions to the perlcritic configuration file to ignore a couple of
warnings that seem acceptable, bringing it in line with the buildfarm's
configuration for those warnings.
Thanks to Andrew for assistance getting my environment set up.
Mike Blackwell
Attachments:
0001-add-returns-to-Perl-files-where-missing.patchtext/x-patch; charset=US-ASCII; name=0001-add-returns-to-Perl-files-where-missing.patchDownload+211-5
On Mon, May 21, 2018 at 08:07:03PM -0500, Mike Blackwell wrote:
This particular patch addresses the warning caused by falling off the end
of a subroutine rather than explicitly returning.
Do we really want to make that a requirement? Making sure that there is
a return clause if a subroutine returns a result makes sense to me, but
making it mandatory if the subroutine does not return anything and
callers don't expect it do is not really a gain in my opinion. And this
maps with any C code.
Just today, I coded a perl subroutine which does not return any
results... This is most likely going to be forgotten.
--
Michael
On Tue, May 22, 2018 at 3:32 AM, Michael Paquier <michael@paquier.xyz>
wrote:
<snip> And this
maps with any C code.
The important differences here are:
*) Declaring a C function as void prevents returning a value. The intent
not to return a value is clear to any caller and is enforced by the
compiler. There is no equivalent protection in Perl.
*) Falling off the end of a C function that returns a type other than
void has undefined results. Perl will always return the value of the last
statement executed.
Because Perl does allow returning a value without explicitly using return,
it's easy to write code that breaks if an unwary person adds a line to the
end of the subroutine. There's a common constructor incantation that has
this problem. It's a real gotcha for C programmers just starting to poke
around in Perl code.
This difference also allows users of .pm modules to abuse the API of a
method intended to be "void", if the value returned falling off the end
happens to seem useful, leading to breakage if the method's code changes in
the future.
This is most likely going to be forgotten.
That's what perlcritic is for. :)
Mike
On Tue, May 22, 2018 at 4:35 PM, Mike Blackwell <maiku41@gmail.com> wrote:
On Tue, May 22, 2018 at 3:32 AM, Michael Paquier <michael@paquier.xyz>
wrote:<snip> And this
maps with any C code.The important differences here are:
*) Declaring a C function as void prevents returning a value. The intent
not to return a value is clear to any caller and is enforced by the
compiler. There is no equivalent protection in Perl.
*) Falling off the end of a C function that returns a type other than void
has undefined results. Perl will always return the value of the last
statement executed.Because Perl does allow returning a value without explicitly using return,
it's easy to write code that breaks if an unwary person adds a line to the
end of the subroutine. There's a common constructor incantation that has
this problem. It's a real gotcha for C programmers just starting to poke
around in Perl code.This difference also allows users of .pm modules to abuse the API of a
method intended to be "void", if the value returned falling off the end
happens to seem useful, leading to breakage if the method's code changes in
the future.This is most likely going to be forgotten.
That's what perlcritic is for. :)
Mike
I should also point out that Mike posted on this subject back on May
11, and nobody but me replied.
And yes, the idea is that if we do this then we adopt a perlcritic
policy that calls it out when we forget.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-May-23, Andrew Dunstan wrote:
And yes, the idea is that if we do this then we adopt a perlcritic
policy that calls it out when we forget.
If we can have a buildfarm animal that runs perlcritic that starts green
(and turns yellow with any new critique), with a config that's also part
of our tree, so we can easily change it as we see fit, it should be
good.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, May 23, 2018 at 1:45 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
On 2018-May-23, Andrew Dunstan wrote:
And yes, the idea is that if we do this then we adopt a perlcritic
policy that calls it out when we forget.If we can have a buildfarm animal that runs perlcritic that starts green
(and turns yellow with any new critique), with a config that's also part
of our tree, so we can easily change it as we see fit, it should be
good.
Should be completely trivial to do. We already have the core
infrastructure - I added it a week or two ago.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 05/23/2018 02:00 PM, Andrew Dunstan wrote:
On Wed, May 23, 2018 at 1:45 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:On 2018-May-23, Andrew Dunstan wrote:
And yes, the idea is that if we do this then we adopt a perlcritic
policy that calls it out when we forget.If we can have a buildfarm animal that runs perlcritic that starts green
(and turns yellow with any new critique), with a config that's also part
of our tree, so we can easily change it as we see fit, it should be
good.Should be completely trivial to do. We already have the core
infrastructure - I added it a week or two ago.
Not quite trivial but it's done - see
<https://github.com/PGBuildFarm/client-code/commit/92f94ba7df8adbcbdb08f0138d8b5e686611ba1f>.
crake is now set up to run this - see
<https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=crake&dt=2018-05-26%2014%3A32%3A19&stg=perl-check>
So, are there any other objections?
The patch Mike supplied doesn't give us a clean run (at least on the
machine I tested on), since it turns down the severity level to 4 but
leaves some items unfixed. I propose to enable this policy at level 5
for now, and then remove that when we can go down to level 4 cleanly,
and use its default setting at that stage.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-May-26, Andrew Dunstan wrote:
Not quite trivial but it's done - see <https://github.com/PGBuildFarm/client-code/commit/92f94ba7df8adbcbdb08f0138d8b5e686611ba1f>.
crake is now set up to run this - see <https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=crake&dt=2018-05-26%2014%3A32%3A19&stg=perl-check>
So, are there any other objections?
The patch Mike supplied doesn't give us a clean run (at least on the machine
I tested on), since it turns down the severity level to 4 but leaves some
items unfixed. I propose to enable this policy at level 5 for now, and then
remove that when we can go down to level 4 cleanly, and use its default
setting at that stage.
Just to be clear -- this is done, right? And we plan no further
enhancements in perlcritic area for pg11?
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 06/11/2018 12:34 PM, Alvaro Herrera wrote:
On 2018-May-26, Andrew Dunstan wrote:
Not quite trivial but it's done - see <https://github.com/PGBuildFarm/client-code/commit/92f94ba7df8adbcbdb08f0138d8b5e686611ba1f>.
crake is now set up to run this - see <https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=crake&dt=2018-05-26%2014%3A32%3A19&stg=perl-check>
So, are there any other objections?
The patch Mike supplied doesn't give us a clean run (at least on the machine
I tested on), since it turns down the severity level to 4 but leaves some
items unfixed. I propose to enable this policy at level 5 for now, and then
remove that when we can go down to level 4 cleanly, and use its default
setting at that stage.Just to be clear -- this is done, right? And we plan no further
enhancements in perlcritic area for pg11?
Yes, modulo
/messages/by-id/f3c12e2c-618f-cb6f-082b-a2f604dbe73f@2ndQuadrant.com
I am hoping that we can get the perlcritic severity down to level 3 with
a combination of policy settings and code remediation during the release
12 dev cycle.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services