Skip to content

Commit

Permalink
Implement optional parameters with OP_MULTIPARAM; use the same SvPADS…
Browse files Browse the repository at this point in the history 
…TALE flag trick that XS-Parse-Sublike uses
  • Loading branch information
@leonerd
leonerd committed Nov 16, 2024
1 parent 1734b47 commit c18f7d0
Show file tree
Hide file tree
Showing 5 changed files with 160 additions and 24 deletions.
6 changes: 4 additions & 2 deletions dump.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1446,6 +1446,7 @@ S_do_op_dump_bar(pTHX_ I32 level, UV bar, PerlIO *file, const OP *o)
case OP_ARGDEFELEM:
case OP_ENTERTRY:
case OP_ONCE:
case OP_PARAMTEST:
S_opdump_indent(aTHX_ o, level, bar, file, "OTHER");
S_opdump_link(aTHX_ o, cLOGOPo->op_other, file);
break;
Expand Down Expand Up @@ -1567,6 +1568,7 @@ S_do_op_dump_bar(pTHX_ I32 level, UV bar, PerlIO *file, const OP *o)
{
struct op_multiparam_aux *aux = (struct op_multiparam_aux *)cUNOP_AUXo->op_aux;
UV nparams = aux->params;
UV nparams_mandatory = nparams - aux->opt_params;
if(aux->opt_params)
S_opdump_indent(aTHX_ o, level, bar, file, "PARAMS = %" UVuf " .. %" UVuf "\n",
nparams - aux->opt_params, nparams);
Expand All @@ -1575,8 +1577,8 @@ S_do_op_dump_bar(pTHX_ I32 level, UV bar, PerlIO *file, const OP *o)
nparams);

for(Size_t i = 0; i < nparams; i++)
S_opdump_indent(aTHX_ o, level, bar, file, " PARAM [%zd] PADIX = %" UVuf "\n",
i, aux->param_padix[i]);
S_opdump_indent(aTHX_ o, level, bar, file, " PARAM [%zd] PADIX = %" UVuf "%s\n",
i, aux->param_padix[i], i >= nparams_mandatory ? " OPT" : "");

if(aux->slurpy)
S_opdump_indent(aTHX_ o, level, bar, file, "SLURPY = '%c' PADIX = %" UVuf "\n",
Expand Down
131 changes: 112 additions & 19 deletions peep.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1066,13 +1066,10 @@ S_maybe_multiparam(pTHX_ OP *o)
struct op_argcheck_aux *argcheck_aux = (struct op_argcheck_aux *)cUNOP_AUXo->op_aux;

UV nparams = argcheck_aux->params;

/* FOR NOW we don't support optional args */
if(argcheck_aux->opt_params)
return;
UV nparams_mandatory = nparams - argcheck_aux->opt_params;

/* Now we should expect to see 'params' count of COP/ARGELEM pairs. Check
* we have each, and **TODO** for now, none of them have an ARGDEFELEM
* we have each.
*/
OP *final_argelem = NULL;
for(UV parami = 0; parami < nparams; parami++) {
Expand All @@ -1082,9 +1079,22 @@ S_maybe_multiparam(pTHX_ OP *o)
if(o->op_type != OP_ARGELEM)
return;

/* FOR NOW we don't support args with defaulting expressions */
if(o->op_flags & OPf_STACKED)
return;
if(parami < nparams_mandatory) {
if(o->op_flags & OPf_STACKED)
return;
}
else {
if(!(o->op_flags & OPf_STACKED))
return;
if(!(o->op_flags & OPf_KIDS))
return;

OP *defelem = cUNOPo->op_first;
if(defelem->op_type != OP_ARGDEFELEM)
return;
if(!(defelem->op_flags & OPf_KIDS))
return;
}

final_argelem = o;
}
Expand Down Expand Up @@ -1123,12 +1133,68 @@ S_maybe_multiparam(pTHX_ OP *o)
aux->slurpy = argcheck_aux->slurpy;
aux->slurpy_padix = 0;

o = argcheck;
OP *paramtests = NULL;

o = OpSIBLING(argcheck);
for(UV parami = 0; parami < argcheck_aux->params; parami++) {
o = OpSIBLING(o);
SKIP_COP(o);
OP *cop_for_param = NULL;
if(OP_TYPE_IS(o, OP_NEXTSTATE) || OP_TYPE_IS(o, OP_DBSTATE)) {
cop_for_param = o;
o = OpSIBLING(o);
}

OP *onext = OpSIBLING(o);

PADOFFSET padix = aux->param_padix[parami] = o->op_targ;

aux->param_padix[parami] = o->op_targ;
if(parami >= nparams_mandatory) {
/* This is optional param */

/* We'll have to capture the defaulting expression subtree and
* rewrite it somewhat
*
* o is currently OP_ARGELEM[ OP_ARGDEFELEM[ other: default-expr ] ]
*
* We need to generate OP_PARAMTEST[ other: OP_PARAMSTORE[ default-expr ] ]
*
* We can't just slice it apart and rebuild it because the
* ->op_next pointers in the default-expr fragment are already set
* to point out at OP_ARGELEM.
*
* What we can do though is steal the existing OP_ARGELEM to be our
* new OP_PARAMSTORE by changing its optype. That will allow us to
* reuse the existing defexpr without disturbing these pointers.
*/
OP *defelem = cUNOPo->op_first;
OP *defexpr = cLOGOPx(defelem)->op_first;
OP *defexpr_start = cLOGOPx(defelem)->op_other;
cLOGOPx(defelem)->op_first = NULL;
cLOGOPx(defelem)->op_other = NULL;
defelem->op_flags &= ~OPf_KIDS;

OP *paramstore = o;
OpTYPE_set(paramstore, OP_PARAMSTORE);
OpLASTSIB_set(paramstore, NULL); /* temporarily to slice it out */
cUNOPx(paramstore)->op_first = defexpr;
paramstore->op_flags |= OPf_KIDS;
paramstore->op_targ = padix;
paramstore->op_next = NULL;
OpLASTSIB_set(defexpr, paramstore);

/* We can't easily just use newLOGOP() here either, because of
* more ->op_next issues. We'll have to fix them up later. */
OP *paramtest = (OP *)alloc_LOGOP(OP_PARAMTEST, paramstore, defexpr_start);
paramtest->op_flags |= OPf_WANT_VOID;
paramtest->op_targ = padix;
OpLASTSIB_set(paramstore, paramtest);

paramtests = op_append_elem(OP_LINESEQ, paramtests,
cop_for_param);
paramtests = op_append_elem(OP_LINESEQ, paramtests,
paramtest);
}

o = onext;
}

if(argcheck_aux->slurpy) {
Expand All @@ -1144,17 +1210,44 @@ S_maybe_multiparam(pTHX_ OP *o)
* and replace it with this single OP_MULTIPARAM
*/
OpMORESIB_set(cop_before_argcheck, multiparam);
OpMORESIB_set(multiparam, cop_after_args);
cop_before_argcheck->op_next = multiparam;

OP *tail = multiparam;

if(final_argelem)
OpLASTSIB_set(final_argelem, NULL);
if(paramtests) {
for(OP *kid = cLISTOPx(paramtests)->op_first, *nextkid; kid; kid = nextkid) {
nextkid = OpSIBLING(kid);
OpMORESIB_set(tail, kid);

/* tail is either a OP_PARAMTEST or a COP */
if(tail->op_type == OP_PARAMTEST)
/* set ->op_next of both OP_PARAMTEST and OP_PARAMSTORE */
tail->op_next = cLOGOPx(tail)->op_first->op_next = kid;
else
tail->op_next = kid;

tail = kid;
}

/* Free the (now-empty) temporary lineseq */
cLISTOPx(paramtests)->op_first = NULL;
cLISTOPx(paramtests)->op_last = NULL;
paramtests->op_flags &= ~OPf_KIDS;
op_free(paramtests);
}

OpMORESIB_set(tail, cop_after_args);

if(tail->op_type == OP_PARAMTEST)
/* set ->op_next of both OP_PARAMTEST and OP_PARAMSTORE */
tail->op_next = cLOGOPx(tail)->op_first->op_next = next_after_args;
else
OpLASTSIB_set(argcheck, NULL);
tail->op_next = next_after_args;

cop_before_argcheck->op_next = multiparam;
multiparam->op_next = next_after_args;
/* TODO: throw away the entire old OP_ARG* sequence
* We have to be really careful due to all the reuse of ops though */

/* TODO: throw away the entire old OP_ARG* sequence */
op_dump(multiparam);
}


Expand Down
30 changes: 27 additions & 3 deletions pp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7805,6 +7805,7 @@ PP(pp_multiparam)
{
struct op_multiparam_aux *aux = (struct op_multiparam_aux *)cUNOP_AUX->op_aux;
UV nparams = aux->params;
UV nparams_mandatory = nparams - aux->opt_params;
char slurpy = aux->slurpy;
AV *defav = GvAV(PL_defgv); /* @_ */

Expand All @@ -7814,12 +7815,21 @@ PP(pp_multiparam)
S_check_argc(aTHX_ argc, nparams, aux->opt_params, slurpy);

UV parami;
for(parami = 0; parami < nparams; parami++, argc--) {
for(parami = 0; parami < nparams; parami++) {
SV **padentry = &PAD_SVl(aux->param_padix[parami]);
save_clearsv(padentry);

if(!argc) {
/* Ran out of arg values for this param. It must be a missing
* optional. Remark that it's missing so a subsequent OP_PARAMTEST
* knows */
SvPADSTALE_on(*padentry);
continue;
}

SV **valp = av_fetch(defav, parami, FALSE);
SV *val = valp ? *valp : &PL_sv_undef;
argc--;

assert(TAINTING_get || !TAINT_get);
if (UNLIKELY(TAINT_get) && !SvTAINTED(val))
Expand Down Expand Up @@ -7883,12 +7893,26 @@ PP(pp_multiparam)

PP(pp_paramtest)
{
croak("TODO pp_paramtest");
dTARGET;

bool ok = TARG && !SvPADSTALE(TARG);

if(!ok)
return cLOGOP->op_other;

return PL_op->op_next;
}

PP(pp_paramstore)
{
croak("TODO pp_paramstore");
dSP;
dTARGET;
SV *value = POPs;

SvPADSTALE_off(TARG);
SvSetMagicSV(TARG, value);

RETURN;
}

PP_wrapped(pp_isa, 2, 0)
Expand Down
9 changes: 9 additions & 0 deletions t/op/signatures_faster.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,15 @@ fails_ok( sub { p2("a1") }, qr/^Too few arguments for subroutine 'main::p2' \(go
fails_ok( sub { p2("a1","a2","a3") }, qr/^Too many arguments for subroutine 'main::p2' \(got 3; expected 2\) at /,
'p2 on three arguments' );

# two params, one is optional
sub p2o1 ( $x, $y = 100 ) { return "P2O1-$x-$y"; }
is( p2o1("a1"), "P2O1-a1-100", 'p2o1(1) OK' );
is( p2o1("a1", "a2"), "P2O1-a1-a2", 'p2o1(2) OK' );
fails_ok( sub { p2o1() }, qr/^Too few arguments for subroutine 'main::p2o1' \(got 0; expected at least 1\) at /,
'p2o1 on zero arguments' );
fails_ok( sub { p2o1("a1","a2","a3") }, qr/^Too many arguments for subroutine 'main::p2o1' \(got 3; expected at most 2\) at /,
'p2o1 on three arguments' );

# with slurpy array
sub p1sa ( $x, @rest ) {
return "P1SA-$x+" . join( '+', @rest );
Expand Down
8 changes: 8 additions & 0 deletions t/perf/opcount.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1038,6 +1038,14 @@ test_opcount(0, "foreach 2 lexicals on builtin::indexed",
argelem => 0,
});

test_opcount(0, "Two-arg one-optional subroutine uses OP_MULTIPARAM",
sub ($x, $y = "default") { return; },
{
multiparam => 1,
argcheck => 0,
argelem => 0,
});

test_opcount(0, "One-arg plus slurpy array subroutine uses OP_MULTIPARAM",
sub ($x, @rest) { return; },
{
Expand Down

0 comments on commit c18f7d0

Please sign in to comment.