Skip to content

op.c auto-use attributes.pm; newSVpvs("attributes") -> newSVpvs_share() #23424

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: blead
Choose a base branch
from
Open

op.c auto-use attributes.pm; newSVpvs("attributes") -> newSVpvs_share() #23424

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions op.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3888,7 +3888,7 @@ S_apply_attrs(pTHX_ HV *stash, SV *target, OP *attrs)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit message is not comprehensible to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a suggestion something better than my wording of "op.c auto-use attributes.pm;"?

I have no preference on the wording of the commit title. Its for future ppl who are not me to read.

There are 3 different places in op.c that do C level equivalent of PP use attributes; or require attributes; every time they execute

void Perl_apply_attrs_string(pTHX_ const char *stashpv, CV *cv, const char *attrstr, STRLEN len)
void S_apply_attrs_my(pTHX_ HV *stash, OP *target, OP *attrs, OP **imopsp)
void S_apply_attrs(pTHX_ HV *stash, SV *target, OP *attrs)

What would you call the use attributes; from C feature in them? I can't think of anything better than "auto-use" and just writing "use attributes" is too confusing, PP level? C level? the keyword "use" or "use the attributes API"?

I know "auto-use Foo.pm;" means somewhere Perl_load_module() is going to be called. But I think nobody knows what Perl_load_module() is or does off the top of their head. Its super rare for me see Perl_load_module() anywhere in core or CPAN XS.

I believe its not a day 1 Perl API call and was added in the 5.6.x era or 5.9.x or 5.10.x or 5.12.x era, because predecessor void Perl_require_pv(pTHX_ const char *pv) was a very quick hack job that wakes up the yylexer API.

perl5/perl.c

Line 3517 in 94b9bf6

Perl_require_pv(pTHX_ const char *pv)

and I think XS devs are afraid of Perl_load_module() because it is the 1 and only function call in the entire Public API takes over ownership of the callers SV*, excluding very obvious and very basic hv_store()/av_store() calls.

Perl_load_module() is also "too cool for school" to a CPAN XS person, since its C code looks like the output of B::ByteLoader or the output of B::C or B::CC.

perl5/op.c

Line 8471 in 94b9bf6

modname = newSVOP(OP_CONST, 0, name);

I dont mind it being copy pasted "B:CC" codegen, its a smart and faster design choice than parsing an ASCII string . But Perl_load_module() scares off 90% of high skill set CPAN XS/C devs, IDK why, Perl_load_module() just does. So b/c is so rarely used by Perl XS ppl, I didn't want to put its name in a commit title.

Perl C ppl don't remember what it does off the top of their head, so I would be making a future reader who reads the title, searching for "what commit could be causing my Zoo bug in the interp's Foo subsystem?", waste time reading its diff code, or just reading the title only and then spend 30 seconds greping perlapi and perlguts to remember what it actually does. I know everyone knows what use attributes; does and can keep scrolling the git log with the mouse wheel. Commit title op.c: Perl_load_module(): newSVxxv() -> newSVyyv() is useless without opening it.

So I thought a "PP" worded title like "op.c auto-use attributes.pm; newSVpvs("attributes") -> newSVpvs_share()" is much better than writing a title like "op.c Perl_load_module(), newSVpvs("attributes") -> newSVpvs_share()" which nobody knows what on earth this commit is doing inside of op.c without opening it because op.c is as huge as beach but with code instead of sand and every commit is the size of dime.

I think "auto-use attributes.pm;" best describes the "location" or "area" of op.c being touched with GPS level accuracy, writing

op.c: Perl_apply_attrs_string S_apply_attrs_my S_apply_attrs: Perl_load_module: newSVpvs -> newSVpvs_share

is too long to be a commit title and unreadable except for the most hard core C devs at P5P, and I try to consider readability of a title for non-C ppl so `"op.c: auto-use attributes;" sounds friendly to read vs the way too list of C symbols above.

Perl_load_module(
aTHX_ PERL_LOADMOD_IMPORT_OPS,
newSVpvs(ATTRSMODULE),
newSVpvs_share(ATTRSMODULE),
NULL,
op_prepend_elem(OP_LIST,
newSVOP(OP_CONST, 0, stashsv),
Expand All @@ -3903,7 +3903,7 @@ STATIC void
S_apply_attrs_my(pTHX_ HV *stash, OP *target, OP *attrs, OP **imopsp)
{
OP *pack, *imop, *arg;
SV *meth, *stashsv, **svp;
SV *meth, *stashsv, *attrpkg, **svp;

PERL_ARGS_ASSERT_APPLY_ATTRS_MY;

Expand All @@ -3914,17 +3914,25 @@ S_apply_attrs_my(pTHX_ HV *stash, OP *target, OP *attrs, OP **imopsp)
target->op_type == OP_PADHV ||
target->op_type == OP_PADAV);

attrpkg = newSVpvs_share(ATTRSMODULE);
/* no sv_2mortal() or its freed by time of leave_scope() -> replace_sv() */
SAVEFREESV(attrpkg);

/* Ensure that attributes.pm is loaded. */
/* Don't force the C<use> if we don't need it. */
svp = hv_fetchs(GvHVn(PL_incgv), ATTRSMODULE_PM, FALSE);
if (svp && *svp != &PL_sv_undef)
NOOP; /* already in %INC */
else
Perl_load_module(aTHX_ PERL_LOADMOD_NOIMPORT,
newSVpvs(ATTRSMODULE), NULL);
else {
/* Can't use 1 SV* head with 2 refcounts for attrpkg.
Perl_load_module()'s callee will modify the buf with sv_catpvs(".pm"). */
SV * sv = newSVhek(SvSHARED_HEK_FROM_PV(SvPVX(attrpkg)));
Perl_load_module(aTHX_ PERL_LOADMOD_NOIMPORT, sv, NULL);
}

/* Need package name for method call. */
pack = newSVOP(OP_CONST, 0, newSVpvs(ATTRSMODULE));
SvREFCNT_inc_simple_void_NN(attrpkg);
pack = newSVOP(OP_CONST, 0, attrpkg);

/* Build up the real arg-list. */
stashsv = newSVhek(HvNAME_HEK(stash));
Expand Down Expand Up @@ -3989,7 +3997,7 @@ Perl_apply_attrs_string(pTHX_ const char *stashpv, CV *cv,
}

Perl_load_module(aTHX_ PERL_LOADMOD_IMPORT_OPS,
newSVpvs(ATTRSMODULE),
newSVpvs_share(ATTRSMODULE),
NULL, op_prepend_elem(OP_LIST,
newSVOP(OP_CONST, 0, newSVpv(stashpv,0)),
op_prepend_elem(OP_LIST,
Expand Down
Loading