Re: [perl #118091] [GITHUB] Split gv_fetchpvn_flags into smaller functions

On Sun, Jul 7, 2013 at 3:19 PM, Father Chrysostomos via RT <
perlbug-followup@perl.org> wrote:

> On Tue May 21 08:23:20 2013, Hugmeir wrote:
> > This is a bug report for perl from fraserbn@gmail.com,
> > generated with the help of perlbug 1.39 running under perl 5.16.2.
> >
> >
> > -----------------------------------------------------------------
> > [Please describe your issue here]
> >
> > https://github.com/Hugmeir/utf8mess/commits/split_gv_fetchpvn_flags
> >
> > That branch splits gv_fetchpvn_flags() into four helper functions:
> > parse_gv_stash_name, which parses the name passed in to find a glob
> > name
> > and optionally a stash, find_default_stash, which, if the previous
> > function
> > didn't find a stash or some flags were passed in, looks for a suitable
> > stash
> > for the new glob, magicalize_gv, which adds magic to a newly-created
> > glob,
> > and
> > maybe_multimagic_gv, which checks if an already-existing glob needs a
> > different
> > king of magic, ala *! needing to be magicalized for $! and %!.
> > ...I'm not confident the names are all that descriptive, so feel free
> > to
> > suggest alternatives for those.
> >
> > The branch also adds some comments to make better sense of how it
> > all works.  They are nowhere near comprehensive and need a second
> > pair of eyes for correctness, but I think that anything that clarifies
> > gv_fetchpvn_flags is a good step forward.
>
> Thank you.  But....
>
>     else if (parse_gv_stash_name(&stash, &gv, &name, &len, nambeg,
> full_len, is_utf8, add)) {
>
> Eight-member parameter lists confuse me even more than large functions.
>  If I am alone in this, fine.
>
> I realise I am guilty of doing this, too, but solely to avoid code
> duplication.
>

Eh.. I think that three of those parameters could be dropped if the
function is reworked a bit, but it would mean duplicating some declarations
in both functions* and an even more convoluted return value. If someone
else speaks up I can look into it, but I would rather leave it as-is for
now and then get rid of the unneeded bits when the function gets a good
refactoring.


>
> I think splitting less of the next part into find_default_stash and
> having the latter (with a different name) just return a boolean
> indicating whether an unqualified symbol belongs in the main package
> would not only be clearer but also provide a useful API function for
> modules to use, i.e., gv_is_in_main(name, namelen, utf8).  (The utf8
> flag would, of course, be ignored, but should stay for completeness’
> sake and possible future compatibility.)
>

This is a good idea -- I've gone and added the function, but left it
private for now, since my aim with this branch is making gv_fetchpvn_flags
easier to maintain (& more personally, understand), rather than improving
the API.


>
> I do *not* think magicalize_gv (should be named gv_magicalize) should be
> adding the GV to the stash.  That is gv_fetchpvn_flags’ job.


Yes, this is spot on. With a bit of tweaking we can get around that, by
inserting the GV into the stash before calling gv_magicalize if addmg is
true and at least one slot in the GV is in use. Downside is that it has to
duplicate a "is this gv empty" check if addmg is true before & after
calling gv_magicalize, but overall it doesn't look too terrible.


>  Also,
> having a gv_magicalize funcion that just vivifies members of the GV
> passed in would be a useful API function.
>

Hm, I'm not so sure about this one. What would be the use case?


>
> I know addmg complicates things.  The problem is that Errno and
> Tie::Hash::NamedCapture tie the variable when loaded, so the GV must
> have been added to the stash already.  arybase just copied those two.
> Maybe we can change those modules not to do that, and have
> gv_fetchpvn_flags tie the variable or pass the SV to the modules and
> have them act on that instead of looking it up via a recursive
> gv_fetchpvn_flags call.
>
> (Maybe leave the addmg parameter in for now and deal with that last
> paragraph later.)
>

Changing the modules sounds like the way to go, but it's not entirely
necessary with the latest changes. I've left addmg in for now since it's
still used to check when to decrease the refcount of the temporary GV that
gv_fetchpvn_flags sometimes creates, and for the special case of a gv being
"empty" but SvMAGICAL(GvSV(gv)) returning true (which is not tested in the
core, so maybe that can't actually happen).


[*] Would this have an impact on the bonus gained from inlining? I get the
feeling that yes, but I know next to nothing about compilers, or inlining,
or compilers and inlining.

• Re: [perl #118091] [GITHUB] Split gv_fetchpvn_flags into smaller functions by Brian Fraser