Add asserts for aTHX, including ability to specify NULLOK#24257
Add asserts for aTHX, including ability to specify NULLOK#24257khwilliamson wants to merge 10 commits intoPerl:bleadfrom
Conversation
khwilliamson
changed the title
khwilliamson
force-pushed
the
Hflag
branch
2 times, most recently
from
a0fb1ab to
141f317
Compare
|
This PR feels to large to review properly, would it be possible to split it up?
I'm not sure that feels like the right solution for this problem to be honest. |
|
#24270 is the first set of non-trivial patches from this. My original idea was to add flags to specify what to do about aTHX. And then, it occurred to me that C99 gives us the flexibility to add things to these macros besides asserts. In particular, the UNUSED attribute could be added to an argument in an embed.fnc entry, and the corresponding PERL_UNUSED_ARG removed from the function source, as regen/embed.pl would automatically add that call to the ASSERT for the function. It's a minor thing in a way, but it seems much cleaner to me to do it this way. The UNUSED calls in the function distract from the logic flow, and are there just because of compiler warnings. Adding a single word attribute to an entry in embed.fnc seems better to me, also in places leading to the removal of #ifdef's in the function source. And then I noticed a significant number of PERL_UNUSED_CONTEXT lines as well. The same logic could get rid of most of them. But then that's another flag to memorize if we did it via flags. And what new ideas might someone have that would apply to the aTHX argument that would require new flags? So, this seemed the best approach for a solution that would be easy to extend in the future. But maybe someone has a different idea, or disagrees that getting rid of these source lines is a good thing. |
Remove a few trailing blanks, and fix a dangling {
khwilliamson
changed the title
This fleshes out some things that hadn't been explained.
These two lines are repeated in an #else. Move them to before the #if, so they work for both cases.
This code was erroneously generating entries in proto.h assuming that they do. This actually only affected three macros: invlist_(subtract|intersection|union)
This moves most of the code that deals with context to a single block, and turns a ternary into code that I think is a bit clearer. And the new scheme will be useful in future commits.
The aTHX parameter is added to the list of parameters in preparation for future commits which will want to do things with it like what is done with the other parameters. The parameter number must be adjusted down in two cases to compensate, and the context parameter must be removed from the list before the rest are all joined together.
This moves a push to a later block, in preparation for generalizing if it is to be done
This makes sure in DEBUGGING mode that the context parameter is not NULL. In non-DEBUGGING mode, we already have an __attribute__nonnull__. This makes the two modes consistent.
Function entries in embed.fnc may now include data for the implicit aTHX parameter, as the first parameter in the entry. If omitted, everything works as always. But including it allows you to say that the aTHX parameter on builds that have them may point to NULL. The next commit will use this new capability.
It is not that uncommon for a function to need aTHX only in order to be able to raise diagnostics for rarely encountered error cases. This commit changes several of the diagnostic functions so that they can be called with a NULL aTHX, and they will extract it themselves. This allows a function that doesn't have an implicit context parameter to not have to extract it; it's only extracted when needed. The immediate impetus of this commit was converting an API function (which hence can't be changed) without thread context to calling another function that has thread context, but it only needs that for raising diagnostics. The only downside is that there is an extra conditional executed within these diagnostic-raising functions that wouldn't have to be there for the normal case. But it is branch-predicted, and raising diagnosics is expensive anyway, so this is a negligible addition. If this mechanism had been in place, the 'nocontext' functions we have could have been simple macros. But people actually do pass pointers to some of these functions, so you can't ever eliminate a public function. I'm trying out a reordering in this commit of the argument portion of the embed.fnc entries. I'm making the NULLOK be postfix to the argument. That puts the important part first. The code has always accepted the keywords anywhere, but the practice has been to put them first. But these are modifiers; the important part is the actual argument.
2 participants
This PR adds the ability to optionally include the aTHX parameter in embed.fnc entries so that you can say that a function accepts a NULL aTHX. It adds an assert that aTHX is not NULL for each function that doesn't have that override. This syntax was chosen so that the aTHX argument can have more attributes attached to it in future commits.
It changes a few low level functions to accept a NULL aTHX.