Add missing ARG_ASSERTs; fix .t that wrongly passed#24270
Merged
khwilliamson merged 7 commits intoPerl:bleadfrom Mar 14, 2026
Merged
Add missing ARG_ASSERTs; fix .t that wrongly passed#24270khwilliamson merged 7 commits intoPerl:bleadfrom
khwilliamson merged 7 commits intoPerl:bleadfrom
Conversation
It was 'M', which is wrong; 'm' is what is meant. 'M' is for when the implementation is via a macro of a different name.
The normal syntax for the beginning of a function in our C code is to
have the return type on the first line, the function name and parameters
on the second line, and the third line containing a single '{', like so:
static void
S_foo(pTHX_ int a)
{
This commit brings the outliers into compliance with this, except for
some machine generated files.
This makes it easier to grep these.
I grepped through the source to add the ones that are generated but don't appear. The intent has been that a Porting test warning gets generated for missing ones that actually have some effect, but no warning for empty ones. However, there is a bug in that test that failed to notice more than a few non-empty ones. This commit adds those; the next few commits will fix the test, so this doesn't happen again. While we're at it, this commit also adds calls to the empty ones. They could become non-empty at any time via changes to the ASSERT macro generation scheme.
These two patterns were meant to differ only in that one applied to .c files, and one to inline headers. But the latter did not contain the guard that compensated for this test file being run inside t/ vs not. And it turns out perlstatic.h also contains a function definition.
The first bug was that it didn't realize there could be spaces in the components of '#define'. Hence this missed finding some ARGS_ASSERT definitions The second bug was that it did not allow for comments on an ARGS_ASSERT usage line, so it missed finding some uses of these.
A recent commit has added all missing ARGS_ASSERT macros, even when currently empty. It's best to always have them. Change so any future functions will need to include one.
Contributor
@khwilliamson I noticed this error output at the end of Would this be addressed by the changes in this ticket? |
Leont
approved these changes
Mar 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds calls for every function missing a generated ARGS_ASSERT macro, fixing the test that was supposed to find missing ones.
Since, now every generated one has a call, it changes the test to fail on even empty ones, so that new functions added to embed.fnc will need to include the ARGS_ASSERT.