Compile anonymous subs as anoncode without srefgen.

[FGasper]

Heretofore the compiler represented an anonymous subroutine as an anoncode op then an srefgen op; the latter’s only purpose was to return a reference to the anoncode’s returned CV.

This changeset slightly optimizes that by making pp_anoncode return a reference if so bidden. The same optimization is applied to pp_anonconst as well.

Hat-tip to @leonerd for the suggestion and guidance.

[atoomic]

I've the feeling that the B::Deparse::pp_refgen also needs to be adjusted as part of your change
for example: $anoncode->first->first->sibling

 sub pp_refgen {
     my $self = shift;
     my($op, $cx) = @_;
     my $kid = $op->first;
     if ($kid->name eq "null") {
     my $anoncode = $kid = $kid->first;
     if ($anoncode->name eq "anonconst") {
         $anoncode = $anoncode->first->first->sibling;
     }
     if ($anoncode->name eq "anoncode"
      or !null($anoncode = $kid->sibling) and
          $anoncode->name eq "anoncode") {
             return $self->e_anoncode({ code => $self->padval($anoncode->targ) });
     } elsif ($kid->name eq "pushmark") {
             my $sib_name = $kid->sibling->name;
             if ($sib_name eq 'entersub') {
                 my $text = $self->deparse($kid->sibling, 1);
                 # Always show parens for \(&func()), but only with -p otherwise
                 $text = "($text)" if $self->{'parens'}
                                  or $kid->sibling->private & OPpENTERSUB_AMPER;
                 return "\\$text";
             }
         }
     }
     local $self->{'in_refgen'} = 1;
     $self->pfixop($op, $cx, "\\", 20);
 }

[FGasper]

I've the feeling that the B::Deparse::pp_refgen also needs to be adjusted as part of your change

Can you expand more on this, please?

I could see removing the anoncode and anonconst sections from there since I think those ops will no longer happen in tandem with srefgen. That would just be pruning, though; as far as I can tell (and as the test suite shows), no change there seems necessary for Perl to work.

[atoomic]

I've the feeling that the B::Deparse::pp_refgen also needs to be adjusted as part of your change

Can you expand more on this, please?

I could see removing the anoncode and anonconst sections from there since I think those ops will no longer happen in tandem with srefgen. That would just be pruning, though; as far as I can tell (and as the test suite shows), no change there seems necessary for Perl to work.

Was looking for consumer of that anoncode / anon post
It looks like that this is now dead code with your change
as a refgen is not holding one anoncode anymore

-					     ->sibling #       srefgen
- 					     ->first   #         ex-list
- 					     ->first   #           anoncode

[FGasper]

It looks like that this is now dead code with your change
as a refgen is not holding one anoncode anymore

Per PMs I’ve replaced the relevant sections of pp_refgen with croak()s.

[leonerd]

You probably want to leave B::Deparse's code in place for deparsing the old optree shapes (give or take some additional comments explaining why), because XS modules that generate optrees may still output that shape for some time to come, until they adopt the new ability. It'd be nice not to break them unnecessarily

[FGasper]

XS modules that generate optrees may still output that shape for some time to come, until they adopt the new ability. It'd be nice not to break them unnecessarily

Reverted, with relevant comments added.

[atoomic]

You probably want to leave B::Deparse's code in place for deparsing the old optree shapes (give or take some additional comments explaining why), because XS modules that generate optrees may still output that shape for some time to come, until they adopt the new ability. It'd be nice not to break them unnecessarily

Sorry I’m confused.

anonconst is experimental and I think the the main advantage of such features is to avoid the deprecation cycle.
So, if we cannot remove that code from B::Deparse as part of this Pull Request.
When will be a good time to remove it?

Regarding anoncode which is not experimental I understand the argument about XS.
You are saying we cannot remove it now, but not sure there would be a better timing in the future too.
Wouldn’t it be hard to audit existing XS packages building optree the previous way?
Adding a comment (as already done) explaining that Perl no longer generates such code seems a good idea.

my 2cts

[leonerd]

Overall the pp_anoncode looks good, but I would apply the same OPf_REF flag to the pp_anonconst as well. Otherwise, what you've done here is changed the shape of how it works without adding the flag; this might have more of a knock-on effect on XS modules that build such optrees themselves. The point of using the flag on OP_ANONCODE was to shield them from that - the same surely should apply to OP_ANONCONST.

[FGasper]

@leonerd Have I resolved your concerns?

[leonerd]

Yeah, looks good now.

[tonycoz]

This introduces some noise at test time:

../lib/B/Deparse-subclass.t .......................................... ok
===(  464165;213  14281/?  185/?  1/? )=================================:const is experimental at (eval 95) line 13, <DATA> chunk 60.
../ext/XS-APItest/t/utf8_warn02.t .................................... ok

which should probably eliminated.

[FGasper]

@tonycoz I’ve updated the branch to remove that warning.

(NB: The warning came from the test I added in the 1st commit, not from the production code changes in the 2nd commit.)

[khwilliamson]

On 9/16/22 16:49, Hugo van der Sanden wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In pp.c <#20290 (comment)>: > @@ -6986,10 +6994,14 @@ PP(pp_anonconst) { dSP; dTOPss; - SETs(sv_2mortal((SV *)newCONSTSUB(SvTYPE(CopSTASH(PL_curcop))==SVt_PVHV - ? CopSTASH(PL_curcop) - : NULL, - NULL, SvREFCNT_inc_simple_NN(sv)))); + + CV* constsub = newCONSTSUB( + SvTYPE(CopSTASH(PL_curcop))==SVt_PVHV ? CopSTASH(PL_curcop) : NULL, + NULL, + SvREFCNT_inc_simple_NN(sv) + ); + + SETs(refto( sv_2mortal((SV *)constsub) )); (Ugh, hate that the email versions of these comments tend to quote extra lines, making it appear to be a conversation about the |newCONSTSUB()| call rather than |SETs()|.) FWIW I have no objection to the extra spaces here - they're correctly balanced, and make the line more readable by giving the eye extra reference points to help it see how to parse the code. In the same way in perl code I'd tend to write |$array[$index]| but |$array[ $subarray[$subindex] ]|. It won't kill me if they are avoided in any specific case - it's really a judgement call when they improve things and when they detract - but I'd hate to see a style guide mandating against them.

+1