Horton

Thread

Commits

  1. Don't fall off the end of perl functions

  1. [PATCH] Clear up perlcritic 'missing return' warning

    Mike Blackwell <maiku41@gmail.com> — 2018-05-22T01:07:03Z

    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

  2. Re: [PATCH] Clear up perlcritic 'missing return' warning

    Michael Paquier <michael@paquier.xyz> — 2018-05-22T08:32:11Z

    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

  3. Re: [PATCH] Clear up perlcritic 'missing return' warning

    Mike Blackwell <maiku41@gmail.com> — 2018-05-22T20:35:10Z

    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

  4. Re: [PATCH] Clear up perlcritic 'missing return' warning

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-05-23T17:38:40Z

    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

  5. Re: [PATCH] Clear up perlcritic 'missing return' warning

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-05-23T17:45:18Z

    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

  6. Re: [PATCH] Clear up perlcritic 'missing return' warning

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-05-23T18:00:18Z

    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

  7. Re: [PATCH] Clear up perlcritic 'missing return' warning

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-05-26T15:06:31Z

    
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

  8. Re: [PATCH] Clear up perlcritic 'missing return' warning

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-06-11T16:34:39Z

    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

  9. Re: [PATCH] Clear up perlcritic 'missing return' warning

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-06-11T16:48:38Z

    
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