Magic v2: Viral value magic#24508
Conversation
leonerd
added
the
defer-next-dev
f6f60ec to
e7256b0
Compare
There's a few commits before those that aren't in #24163 (I wondered where the op.c change came from.) |
| } | ||
|
|
||
| SV *ex = vmess(pat, args); | ||
| PERL_ARGS_ASSERT_VWARN; |
There was a problem hiding this comment.
The assert should be above the vmess() call, it's pointless below it.
There was a problem hiding this comment.
Oh oops. I don't recall moving that but maybe that was a recent(ish) blead fix that has been broken by some rebase operation over the past few months. I shall fix that.
Ahyes, those are some post-phase1 fixups to other files to use the new magic system in |
9d35f64 to
bc28a9c
Compare
|
Since it seems nobody's going to review this while it's drafted I shall undraft it. I don't imagine there's a danger anyone will accidentally merge it ahead of time anyway. ;) |
d2cdd7c to
39731be
Compare
|
Are there any reviewers for this? |
| /* ideally we could just remove the magic from the SV but we don't get the SV here */ | ||
| SvREFCNT_dec(mg->mg_obj); | ||
| mg->mg_obj = NULL; | ||
|
|
||
| return 0; | ||
| MgAUXSV_set(mg, NULL); |
There was a problem hiding this comment.
Ahyes; though that's part of #24547. I'll see what I can do there.
| GCC_DIAG_IGNORE_DECL(-Wmissing-field-initializers); | ||
| static const struct MagicFunctions magicfuncs_perlencoding = { | ||
| .ver = 2, | ||
| .shape = MGv2s_BASE, | ||
| .debug_name = "PerlIO::encoding/tag", | ||
| }; | ||
| GCC_DIAG_RESTORE_DECL; |
There was a problem hiding this comment.
Given the discussion I'm not sure this still needs to be here,
| /* Macros for valuemagic support */ | ||
| /* TODO: allow these to be conditional like the taint ones are */ | ||
| #define VALUEMAGIC_CLEAR \ | ||
| #if defined(NO_VALUEMAGIC_SUPPORT) |
There was a problem hiding this comment.
Should -DNO_TAINT_SUPPORT implicitly -DNO_VALUEMAGIC_SUPPORT? This would simplify checks like in magic2_value.t
It would also later allow value magic without taint
|
Except for the minor issues I mentioned it looks good to me. |
| # | ||
| # unless ($defines =~ m/\b(NO_VALUEMAGIC_SUPPORT)\b/) { | ||
| # my $valuemagic_support = 'define'; | ||
| # s/^(valuemagic_support=['"])(["'])/$1$valuemagic_support$2/m; | ||
| # } |
There was a problem hiding this comment.
I thought there was some discussion of this recently: on the fragility of using compiler -D switches to control this type of feature instead of Configure defines.
But I couldn't find it.
I don't know if you saw it, or if you agree or not... but it seems relevant to this (wishing I could find it)
There was a problem hiding this comment.
Ahyes, I vaguely recalled it. That's why for example, the newly-added variable in intrpvar.h isn't #ifdef-guarded. That way the structure always has the same shape. The only thing that the macro guards are the actual bits of code that implement the behaviour.
If the value magic feature is compiled out, stub implementations of the propagate and unpropagate functions are still built anyway, which just do nothing. So I think it should be fairly safe if, for example, a CPAN module gets built with a different set of flags than the core binary. Structure layouts all match, but the relevant behaviour just won't happen unless all the parts line up.
leonerd
removed
the
defer-next-dev
9660641 to
8e4b7ae
Compare
| /* we can't make this one `static` because -Wc++-compat gets upset if we do */ | ||
| extern const struct ScalarValueMagicFunctions magicfuncs_value; |
There was a problem hiding this comment.
You should be able to predeclare the function instead. Not that it's a big deal.
a0113f8 to
f4e01d1
Compare
The existing `MAGIC` and `MGVTBL` structures are insufficiently extensible, because they lack any embedded type information, or any version numbering by which new abilities can be added in a backwards-compatible way. Commit 6d97c86 managed to free a single bit in the `mg_flags` field; it is this bit that we use to indicate that the `MAGIC` structure refers to version 2 of Magic. The new structures are defined in a way that should be much easier to permit updates in future; primarily by the presence of a version number in the `ver` field, and the set of trigger functions in the `shape` field. In this way, we hope to be able to extend the abilities of magic in the future by adding new structure shapes as well as new trigger functions.
Currently unused by the rest of the code. This commit is extracted separately simply so it can be placed at the very start of the branch. A full rebuild is needed either side of this commit, so by being at the beginning this makes rebase, bisect, and other operations easier as now you don't have to `make clean`.
Adds a new SVs_VMG flag + associated macros; use the SvVMAGICAL flag to fast-path sv_has_valuemagic() if appropriate Use the sv_has_valuemagic() function to know whether to call mg_propagate() in sv_setsv_flags()
Much like -DNO_TAINT_SUPPORT, this is optional and not default, but will disable value magic support in exchange for getting that little extra performance boost by not having to worry about it.
2 participants
(This is a dependent PR that requires first that #24547 be merged)
A currently-draft PR that builds on top of the general "Magic v2" to add viral value magic; as outlined in PPC0036.
Note this PR currently includes all of the commits included in the first phase PR as well, so that the the actual result makes sense. I'll have to rebase it on top of blead once #24547 is merged. For review purposes, it likely makes sense to filter this PR down and look at only the final 3 commits: