regen/embed.pl: Fix, refactor ARGS_ASSERT generation #23502
Merged
Conversation
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
|
Having a quick look it: I guess the issue with the old code was that: should've been something like: Personally I would prefer seeing a commit that first did that as a fix so that it is:
before refactoring the code. (The refactoring should then only update the order of asserts but not add any of them) From the refactoring I also can't tell if the old/new code behaves fully correct/as expected: before it would (PS: small typo in the commit message: "holding arrays and later going through them later..." --> 2 x later?) |
khwilliamson
force-pushed
the
asserts
branch
2 times, most recently
from
840c303 to
189870c
Compare
Yes; I did not feel like tracking down something that was Not My Bug, but you are right. And you tracked it down for me. And the jostling of code showed that it was only by luck that the G flag in embed.fnc was honored in some cases. |
bram-perl
reviewed
Jul 30, 2025
khwilliamson
force-pushed
the
asserts
branch
2 times, most recently
from
84c8c84 to
3b74efd
Compare
khwilliamson
changed the title
bram-perl
reviewed
Jul 30, 2025
A PERL_ARGS_ASSERT macro will be generated for this function in a future commit, and compilation will fail unless the names match.
This has no effect currently, but several commits hence, a bug will be fixed that would otherwise generate a check that actually is inappropriate for these cases.
An ARGS_ASSERT macro is always generated for every function listed in embed.fnc, unless possibly suppressed with the G flag for that entry. The generated macro is empty if there is nothing to assert. It is mandatory (enforced through a porting test) to call that macro when non-empty. (Hopefully the call occurs in the function the macro is designed for, but the porting test is currently simplistic and doesn't check for that; often compilation would fail anyway if it did get placed in the wrong function, as the parameter names the macro expects and the ones in the function could easily not match). It is optional (but a good idea) to call the macro even when empty. That way this commit would not have been necessary. From time to time, an empty macro becomes non-empty as we figure out more things to check for. When that happens, the porting test fails for the newly-non-empty macros that aren't called. If the function had originally called the empty-one, its source wouldn't have to change at all. Several commits from now will make some ARGS_ASSERT macros non-empty; this commit adds calls to the ones that weren't already called.
As suggested by Bram, this was hard to understand
$argname is used inside this 'if' as part of a newly created array element. It needs to be defined for the code to work that later looks at that element. No current calls result in an undefined value for this.
This is in preparation of combining this if clause with the one just below to as many of the conditions in common as possible. In this clause, it applies only to functions, as we don't create ARGS_ASSERT macros for macros. However when something is going into mathoms.c (b flag), it is marked as a macro, but also has a function, so we need to create the ASSERT in this case.
This makes no difference in processing now, but will minimize the 'diff' output of a future commit.
This commit combines two separate 'if' blocks into one big if block with the conditionals in common to the two blocks, and then the two smaller blocks, each with the conditions that apply to it individually. And then everything is reindented to correspond
This macro is actually always generated, but is empty if there are no assertions needed. Before this commit the code incorrectly generated an empty macro when there were no arguments marked NN even when types were defined that would call for asserts to be generated.
This simple change makes the code a bit easier to understand.
Prior to this commit, the code created an array describing an input
argument, adding it to a list of all the arguments, and later went
through that list using a grep to reconstruct some information that had
been lost (because the array didn't include all the needed relevant
information), and placed the result in its final form.
This commit moves the calculation of the final form to earlier, where it
first is encountered. This means all the information is available, and
no grep is needed.
Code review revealed a subtlety here. The moved code has an
if $nullok
but prior to this commit, there were two different $nullok variables,
with slightly different meanings. Prior to this commit, the moved code
was using the second $nullok; afterwards, the first one. Hence, even
though the name is the same, before and after, it is a different
variable, with a (slightly) different meaning.
The second $nullok was populated by the grep, and meant that the
argument did not have either a NN nor a NZ modifier.
The first $nullok meant simply that the arg had a NULLOK modifier.
After the commit the single $nullok means that the arg had a NULLOK
modifier.
This change actually has no effect (as demonstrated by the fact that
this commit doesn't change the generated proto.h). The moved code is
executed only for an argument where $type_asserts{$argtype} exists.
This hash is constant, and has been populated so that only pointer
arguments have entries. NZ is not valid for pointer arguments, so we
know that both before and after this commit, the arg did not have an NZ
modifier. Also it is illegal to specify both NN and NULLOK, so both
before and after this commit, the arg did not have a NN modifier. And
both before and after this commit the arg does have a NULLOK modifier.
These two arrays are combined anyway into a third array later in the code; this just goes with the third array from the start. The order of some assertions in the generated proto.h are changed. But this makes sure that we continue to assert that an argument is present before dereferencing it.
By creating the @asserts array with each element in its final form, the loop that later did this can be eliminated.
This removes a misleading comment. NULLOK now does have some effect. It changes the actual generated assertion for arguments where a type assertion is generated. (This has actually been true since 2463f19) This also vertically aligns some statements for ease of reading, and reorders things to group them together in a block.
bram-perl
approved these changes
Aug 1, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 fixes a bug in which certain conditions failed to generate a required ARGS_ASSERT macro
It refactors the generation in preparation for future commits and changes the embed.fnc G flag effect so that it is overridden for all cases where a non-empty ARGS_ASSERT entry is generated; previously only some cases overrode it.