Skip to content
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.

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

perlapi: Fold in gv_autoload4 to gv_autoload_pv group #22331

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

khwilliamson
Copy link
Contributor

@khwilliamson khwilliamson commented Jun 21, 2024

And correct some of the misleading documentation previously there

@jkeenan
Copy link
Contributor

jkeenan commented Jun 22, 2024

In perl-5.40.0, when I say perldoc perlapi, I see:

    "gv_autoload_pv"
    "gv_autoload_pvn"
    "gv_autoload_sv"
        These each search for an "AUTOLOAD" method, returning NULL if not
        found, or else returning a pointer to its GV, while setting the
        package $AUTOLOAD variable to "name" (fully qualified). Also, if
        found and the GV's CV is an XSUB, the CV's PV will be set to "name",
        and its stash will be set to the stash of the GV.
...
        In "gv_autoload_sv", *namesv is an SV, and the name is the PV
        extracted from that using ""SvPV"". If the SV is marked as being in
        UTF-8, the extracted PV will also be.

            GV *  gv_autoload_pv (HV *stash, const char *namepv, U32 flags)
            GV *  gv_autoload_pvn(HV *stash, const char *name, STRLEN len,
                                  U32 flags)
            GV *  gv_autoload_sv (HV *stash, SV *namesv, U32 flags)

    "gv_autoload4"
        Equivalent to "gv_autoload_pvn".

            GV *  gv_autoload4(HV *stash, const char *name, STRLEN len,
                               I32 method)

Two questions:

My admittedly naive reaction to seeing:

GV *  gv_autoload_pvn(HV *stash, const char *name, STRLEN len, U32 flags)

and

GV *  gv_autoload4(HV *stash, const char *name, STRLEN len, I32 method)

so closely juxtaposed is to think, "How can these be 'equivalent' when they have different 4th arguments?" Can you clarify?

Then, the subject line of your pull request claims that "gv_autoload4 and gv_autoload_pvn are identical" -- i.e., not merely 'equivalent' but 'identical', which sounds like a much stronger claim. Can you clarify? Thanks.

@khwilliamson
Copy link
Contributor Author

It's best to write documentation assuming the reader is more naive than not. Thanks for catching this. I missed this, and it isn't immediately obvious what is actually happening, so I'm looking at it now.

@khwilliamson khwilliamson changed the title perlapi: gv_autoload4 and gv_autoload_pvn are identical perlapi: Fold in gv_autoload4 to gv_autoload_pv group Jun 23, 2024
@khwilliamson
Copy link
Contributor Author

The previous documentation also did not mention what flags are looked at. I wnet throu`gh old commits and the source code to flesh out the pod; and the final parameter of autoload4 is very different than what I had said in the earlier form of this PR

@jkeenan
Copy link
Contributor

jkeenan commented Jun 23, 2024

When I build this branch, say perldoc pod/perlapi.pod and search for GV_AUTOLOAD_ISMETHOD, I do not find it in any "subheading" -- only in the middle of this paragraph:

The "method" parameter in "gv_autoload4" is used only to indicate
that the name is for a method (non-zero), or not (zero). The other
forms use the "GV_AUTOLOAD_ISMETHOD" bit in "flags" to indicate
this.

Similarly, I find SVf_UTF8 in many places, but none of them subheadings. GV_SUPER occurs twice in paragraph texts, but never as a subheading.

Am I misunderstanding what =for apidoc is supposed to do?

(Note that I'm trying to review your p.r.s more for the form in which they finally appear when called with perldoc rather than the content of the paragraphs, which is outside my skill set.)

@khwilliamson
Copy link
Contributor Author

Our documentation is very imperfect, as in almost all products. At least we are volunteers. I could rant about how documentation is under-appreciated by management almost everywhere. For these flag bits, someone could write an entry (patches welcome!), but failing that, having the bit mentioned in the places that look at it is likely adequate, if not ideal.

I added the h flag to apidoc lines some releases ago. It stands for "hidden". It is designed to indicate that this element is documented (to whatever extent) here, but this isn't pod that should go in perlapi or perlintern. If this is a pod file, instead, a link is generated in one of those two files to this one, saying that this is the file where that is discussed. Currently, if it isn't a pod file, only an X<> index entry is generated, so that indexing programs find it, which includes perltoc to a limited extent. You can grep that file for this item and find where its mentioned. What your comment is telling me is that autodoc.pl should generated an entry for GV_SUPER and the others you mentioned in perlapi or perlintern that merely mentions, and links to the =items that they are mentioned under. I'll look into that

@jkeenan
Copy link
Contributor

jkeenan commented Jun 23, 2024

[snip]
What your comment is telling me is that autodoc.pl should generate an entry for GV_SUPER and the others you mentioned in perlapi or perlintern that merely mentions, and links to the =items that they are mentioned under. I'll look into that.

That revision to autodoc.pl sounds like a big project. Perhaps that could be created as an Issue which someone else (a newcomer!) could take on. In the meantime, I'll just keep doing a basic review of your pull requests. Thanks!

gv.c Outdated
Comment on lines 1324 to 1326
The only other significant value in C<flags> currently is C<GV_SUPER>
to indicate, if set, to additionally search for the name in stashes accessible
via C<@ISA>.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incorrect per my comment in #22329, GV_SUPER means to skip searching the specified stash (see perl5180delta)

And correct some of the misleading documentation previously there
@khwilliamson khwilliamson merged commit f651c04 into Perl:blead Jul 11, 2024
29 checks passed
@khwilliamson khwilliamson deleted the autload4_api branch July 11, 2024 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants