Обсуждение: [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
Вложения
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 https://www.postgresql.org/message-id/f3c12e2c-618f-cb6f-082b-a2f604dbe73f%402ndQuadrant.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