Conversation
leonerd
force-pushed
the
sv-flags-tidy
branch
from
3487a9f to
4dbe48c
Compare
sv.h
Outdated
| #define HvAMAGIC(hv) (SvFLAGS(hv) & SVf_AMAGIC) | ||
| #define HvAMAGIC_on(hv) (SvFLAGS(hv) |= SVf_AMAGIC) | ||
| #define HvAMAGIC_off(hv) (SvFLAGS(hv) &=~ SVf_AMAGIC) | ||
| #define assert_HV(sv) assert_(SvTYPE(sv) == SVt_PVHV) |
There was a problem hiding this comment.
why assert_() instead of assert()?
The first was really only added due to a bug in perl's own assert() macro.
There was a problem hiding this comment.
Ah I was just copying the style from the other asserts earlier up in the same file (e.g. assert_not_ROK).
regen/embed.pl
Outdated
| HvMAX | ||
| HvNAME_HEK | ||
| HvNAME_HEK_NN | ||
| HvOVERLOAD_off |
There was a problem hiding this comment.
Ideally to keep this without adding it to the "I'm here for historical reasons", document it.
And HvOVERLOAD_on() since embed.pl is undefining it.
Note "Strive to make this list empty." at the top of this definition.
There was a problem hiding this comment.
I don't want to document it, I don't want anyone to use it. But if I don't put it in here then it isn't visible for other macros to call. I couldn't work out another way to do that.
This exists only so that the older SvAMAGIC_on wrapper still works.
There was a problem hiding this comment.
I can imagine more things coming up in this category soon (e.g. Magic V2, Attributes V2) - the idea of "users shouldn't use this macro so I don't want to document it, but it has to exist so that other macros that are documented can use it".
If it were a C function we could static it and hide it properly, but that concept doesn't work for macros so it just has to remain.
Maybe we could split the whitelist in two sections; the historic "Oops this is visible", from the "exists only as a dependency for others".
sv.h
Outdated
| * no overload methods. Note that this used to be set on the object; but | ||
| * is now only set on stashes. | ||
| */ | ||
| #define SVf_AMAGIC 0x10000000 /* now unused on scalars */ |
There was a problem hiding this comment.
SVf_AMAGIC has some use on CPAN:
distros/B/B-Flags/Flags.xs: if (flags & SVf_AMAGIC) sv_catpv(RETVAL, "OVERLOAD,");
distros/C/Cpanel-JSON-XS/XS.xs: if (!SvSTASH(rv) || !(SvFLAGS(sv) & SVf_AMAGIC)) {
distros/J/JavaBin/JavaBin.c: SvFLAGS(GvSTASH(gv)) |= SVf_AMAGIC;
and other mentions I didn't follow-up on.
There was a problem hiding this comment.
Ah oops; well I guess I'll have to drop that second commit then.
There was a problem hiding this comment.
Actually, I'd still like to stop core from using the old name so I've added a back-compat wrapper for it.
leonerd
force-pushed
the
sv-flags-tidy
branch
2 times, most recently
from
dc9ff0b to
44b29ca
Compare
* Alias the SVf_AMAGIC flag to SVphv_OVERLOAD * Rename the HvAMAGIC set of macros to HvOVERLOAD * Provide back-compatibility wrappers for the old name when !PERL_CORE This flag is only ever used on SVt_PVHV, so it makes sense to name it accordingly. This suggests that the SVf_AMAGIC constant is otherwise unused and might be safe to remove or repurpose for other things.
leonerd
force-pushed
the
sv-flags-tidy
branch
from
44b29ca to
b7a7701
Compare
2 participants
This flag is only meaningful on HVs (moreover, HVs that are stashes), so it would be nicer to have an
SVphv_...name. Also, while the name "AMAGIC" appears in a lot of core source code currently, I'd like to try to gradually nudge it away from being considered a kind of "magic". ((In the very longterm, I'd like to see the "amagic" table disappear in favour of just another field in the STASHAUX structure and therefore not be a kind of magic at all. But I digress...))This change is just the first little step towards that goal, that I encountered on the route to making some other, somewhat unrelated changes to the SvFLAGS surroundings.