(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