| Subject: | inconsistent handling between perl versions of subref argument |
Below is an interesting discussion from #p5p. You may want to add a typemap to make the behaviour more consistent between perl versions, as TonyC suggests.
17:26 <%ether> huh. PadWalker::closed_over('My::Module::subname') works fine in new perl, but not in 5.16.3 and lower.
17:26 <%ether> it seems to want closed_over(\&My::Module::subname) in older perls.
17:26 < mst> that's ... how I've always used it
17:26 <%ether> the error is "PadWalker::closed_over: cv is not a CODE reference"
17:26 <%ether> I don't know where cv is coming from
17:27 < mst> uh, ok, so, what's happening is
17:27 <%ether> mst: I'd used PadWalker so often I didn't bother to RTFM, and using a string worked, so I shipped it.
17:27 < mst> closed_over is documented to take a subref
17:27 < mst> the argument is called cv
17:27 < mst> and whatever internal API it uses must have, in later perls, started accepting a string as well
17:27 < mst> and it's passing the input through to that API without touching it
17:28 <%ether> is this something that ppport.h might possibly paper over?
17:28 * ether finds that whole system a ball of black magic
17:28 <%ether> alh++
17:28 < mst> which is using CvPADLIST
17:29 <%ether> cool
17:29 < mst> which ... what
17:29 < purl> i heard which ... was odd, yes? I mean, if invoking 'perl' gets $PERLLIB into @INC, is CPAN actively _removing_ it?
17:29 < mst> hrm
17:29 <%ether> PadWalker's docs do say subref, otherwise I'd send a doc patch to clarify
17:29 * ether fixes, reships
17:30 < mst> I don't get how that doesn't break
17:30 < mst> uhh
17:30 < mst> maybe the typemap changed for a cv arg?
17:31 * mst boggles
17:31 < mst> yeah, ok, I've no idea why the string version works
17:33 <%ether> I guess I was thinking of things like sort, which take subrefs or subnames interchangably, when I did that
17:33 < mst> yeah, and I'm betting that something got tweaked in how XS is compiled, which is now generating code that does that
17:35 < mst> BINGO
17:36 < mst> ether: http://paste.scsys.co.uk/482445
17:36 < mst> and PPPort does appear to come with some typemap-ish bits already
17:37 < mst> alh: ^^ can PPPort paper over http://paste.scsys.co.uk/482445 ?
17:37 < TonyC> that would be more PadWalker providing a custom typemap
17:37 < mst> TonyC: are you sure? the differences in the core typemaps seem completely sufficient
17:38 < mst> and I don't see a typemap in the padwalker dist
17:38 < TonyC> if PadWalker prefers one of the two behaviours it can provide a custom typemap
17:39 < mst> I think the thought was more that PPPort could perhaps paper over the difference so dists that wanted to could have the newer behaviour; isn't that mostly what it does?
17:40 < TonyC> it provides APIs to older perls that were added to newer perls
17:40 <%ether> is the new behaviour more correct? should stringy subnames be treated as subrefs?
17:40 <%ether> arguably, the behaviour in newer perls isn't doing what PadWalker wants
17:40 < TonyC> the old typemap is using the APIs it uses correctly
17:40 < TonyC> if PadWalker wants the more restrictive form, a typemap is the place to do it
17:42 < mst> well, to my mind, what people use PPPort for is "I want this code to work the same across different perls"
17:42 < TonyC> though the old typemap might be broken for some corner cases, see RT #96872
17:43 < TonyC> which seems to be why the typemap was changed
17:49 <%ether> I wonder if it's worth bisecting to find the commit that changed this
17:49 <%ether> might be worth conferring with the person responsible to see if this was an intended side effect
17:49 < TonyC> ether: 8465c88d32
17:50 <%ether> ah, that's where you got RT#96872 from
17:51 < TonyC> yeah, I looked at a blame of the typemap and worked from there
17:51 <%ether> kk, and perl5180delta.pod has: The "CV *" typemap entry now supports "&{}" overloading and typeglobs, just like "&{...}" [perl #96872].
17:52 <%ether> ergo, a good candidate for supporting in PPPort
17:52 < TonyC> PPPort doesn't provide typemaps though, which is where this is an issue
17:53 < TonyC> unfortunately the perl typemap isn't dual-life
17:53 <%ether> I'll open a ticket for alh to request this, and one for robinh pointing to this conversation and the ppport ticket.
17:53 <%ether> oh? I thought mst was saying it had typemap support
17:53 < TonyC> AFAIK it just generates ppport.h, maybe I'm wrong
17:54 < mst> oh, hang on, maybe I'm confusing that PPPort has to ship its own partial typemap to make the rest of its stuff work
17:57 < TonyC> Imager does something similar - provides a typemap for older perls
In case that nopaste has vanished, its contents were:
5.16.2 lib/ExtUtils/typemap -
T_CVREF
STMT_START {
SV* const xsub_tmp_sv = $arg;
SvGETMAGIC(xsub_tmp_sv);
if (SvROK(xsub_tmp_sv) && SvTYPE(SvRV(xsub_tmp_sv)) == SVt_PVCV)
{
$var = (CV*)SvRV(xsub_tmp_sv);
}
else{
Perl_croak(aTHX_ \"%s: %s is not a CODE reference\",
${$ALIAS?\q[GvNAME(CvGV(cv))]:\qq[\"$pname\"]},
\"$var\");
}
} STMT_END
5.20.2
T_CVREF
STMT_START {
HV *st;
GV *gvp;
SV * const xsub_tmp_sv = $arg;
SvGETMAGIC(xsub_tmp_sv);
$var = sv_2cv(xsub_tmp_sv, &st, &gvp, 0);
if (!$var) {
Perl_croak(aTHX_ \"%s: %s is not a CODE reference\",
${$ALIAS?\q[GvNAME(CvGV(cv))]:\qq[\"$pname\"]},
\"$var\");
}
} STMT_END