develooper Front page | perl.perl5.porters | Postings from July 2021

Review of `defer` (was: FINALLY - ready for comments/review)

Thread Next
From:
Paul "LeoNerd" Evans
Date:
July 28, 2021 13:01
Subject:
Review of `defer` (was: FINALLY - ready for comments/review)
Message ID:
20210728140046.46dc3ad4@shy.leonerd.org.uk
(resurrecting an old thread: 
  https://www.nntp.perl.org/group/perl.perl5.porters/2020/07/msg258096.html
  )

I've finally (pardon the pun) got around to resuming my branch on this.

Since the original review, we've renamed the CPAN module to "defer"
because that seems to match a bunch of other languages. It's also been
implemented and tried out as a CPAN module:

  https://metacpan.org/pod/Syntax::Keyword::Defer


The branch for core perl now lives at:

   https://github.com/leonerd/perl5/tree/defer


On Mon, 27 Jul 2020 11:53:52 +0100
Dave Mitchell <davem@iabyn.com> wrote:

> On Tue, Jul 21, 2020 at 11:47:55PM +0100, Paul "LeoNerd" Evans wrote:
> > It is now ready for general comment and review; I'll make a PR for
> > it soon.  
> 
> Some random comments:
> 
> * using a PVOP - this is bad, and you should feel bad!!!! :-)
> 
> I'd suggest using a LOGOP instead as a somewhat less torturing
> approach. If I understand correctly, the two OP pointers you want to
> hang off the OP_PUSHFINALLY are the root op and start op of the block.
> 
> making logop->op_first point to the root op of the block is "normal"
> and should make everything work just fine: for example at the moment
> 'perl -DX' and 'perl -MO=Concise' don't appear to display the body of
> the block. Similarly, any B-type code will then automatically see the
> block, e.g. if scanning the op tree for some reason.
> 
> The logop->op_other field is a generic pointer to some other op which
> is usually in some way an alternative to op_next under some
> circumstances; using this to store the block start is a slight
> stretch, but will generally work well;

Yup. That's now all done, based on following the same pattern that
worked for Syntax::Keyword::Defer, and List::Keywords.


> you just need to ensure that
> OP_PUSHFINALLY is treated the same as OP_AND etc in places like
> Perl_rpeep so that code within the block gets optimised.
> 
> I'd also suggest adding a test to t/perf/opcount.t that with code like
> FINALLY { $a[0] } there is 1 aelemfast op present, proving that the
> code within the blocjk has been optimised.

Done.


> Similarly test that the block is processed by Perl_finalize_optree
> too, e.g. do eval '{ FINALLY { use strict; foo }' and check that you
> get 'Bareword "foo" not allowed'. Even if it works at the moment, it
> might not in future.

Done


> * why not just "use feature 'finally'" rather than 'finally_block'?

This is now renamed to `defer` anyway.


> * I think pp_pushfinally should be in pp_ctl.c rather than pp.c as
> it's related to flow control - its certainly where I would first look
> for it.

Moved.


> * your branch doesn't current compile under threads.

Huh, most most weird. Apparently you can't use croak() in pp_ctl.c when
under -Dusethreads, but you can without it. Highly strange.

In any case, now fixed.


> * I like the name LEAVE - why was it rejected?

(See above on renaming thoughts)


> * docs: the opening line:
> 
>     A block prefixed by the FINALLY modifier provides a section of
> code which runs at a later time in execution.
> 
> I would change that to
> 
>     ...  at a later time during scope exit.
> 
> to more immediately emphasise what type of beast it is.

Done


> * docs: I would put explicit { }'s around your code examples to
> emphasise that things run on scope exit, as opposed to .. what? ..
> file exit??? etc.

Good idea. 

-- 
Paul "LeoNerd" Evans

leonerd@leonerd.org.uk      |  https://metacpan.org/author/PEVANS
http://www.leonerd.org.uk/  |  https://www.tindie.com/stores/leonerd/

Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About