Fix: simplecpp ## fails to expand function-like macro when '(' is not adjacent - #655
Fix: simplecpp ## fails to expand function-like macro when '(' is not adjacent#655paulafy6 wants to merge 2 commits into
Conversation
|
Thanks for the contribution!
|
|
No need to repeat the whole issue description and what the fix does in the PR message as well. I know AI loves to do that but it often gets out-of-sync if the code in the PR or the issue description changes (it did in this case) and might end up misleading (redundancies are never great). |
| // Only active in the appendTokens context (expandResult==false) to | ||
| // avoid unintended side-effects inside the main expansion loop. | ||
| const Token *scan = nextTok; | ||
| while (scan && sameline(B, scan)) { |
There was a problem hiding this comment.
I believe this scan && is redundant it's done in the sameline function.
| while (scan && sameline(B, scan)) { | |
| while (sameline(B, scan)) { |
| // and is separated from B by a comma in the replacement text. | ||
| // Only active in the appendTokens context (expandResult==false) to | ||
| // avoid unintended side-effects inside the main expansion loop. | ||
| const Token *scan = nextTok; |
There was a problem hiding this comment.
since scan is only used in the while loop, I believe it would be nicer to write a for loop.
ffb123e to
130ee4c
Compare
… adjacent
When the ## operator concatenates two tokens to form a function-like macro
name (e.g. PREFIX_ ## kind → PREFIX_SCALAR), simplecpp looked for the
argument list '(...)' only at B->next. In PAR-style indirection patterns
the '(' is separated from B by a comma or a variadic parameter token:
#define PAR(a, ...) a __VA_ARGS__
#define PREFIX_SCALAR(T, N) T N
#define DISPATCH(kind, ...) PAR(PREFIX_ ## kind, (__VA_ARGS__))
DISPATCH(SCALAR, int, x) // was: [unknownMacro] — now: int x
Because '(' was not found, expansion was aborted and the macro was
reported as unknownMacro, causing cppcheck to skip the entire translation
unit.
Fix: when B->next is not '(' and we are in the appendTokens context
(expandResult==false), walk forward on the same line skipping ',' separators
and resolving named parameter tokens via expandArg(). The first '(' found
(literally or as the head of an expanded argument) is used as lpar and
passed to appendTokens() as before. The forwardScan flag ensures
expandToken() is called on the result even when expandResult is false.
The forward scan is restricted to expandResult==false to avoid unintended
side-effects in the main expansion loop.
…like_par_indirection)
Covers the fix in expandHashHash(): when ## concatenation produces a
function-like macro name but '(' is not immediately adjacent in the
replacement text (hidden behind a comma/parameter), the forward scan
must locate '(' and complete the expansion.
#define PAR(a, ...) a __VA_ARGS__
#define PREFIX_SCALAR(T, N) T N
#define DISPATCH(kind, ...) PAR(PREFIX_ ## kind, (__VA_ARGS__))
DISPATCH(SCALAR, int, x) // expected: int x
130ee4c to
a2f6f52
Compare
|
Sorry for the delay, I just took into account your review and pushed force the changes. |
|
I asked AI to review this. My experience is that AI reviews are almost always wrong, so take it with a mountain of salt. But please look at it and write your response.. What it claims to fix: when ## concatenates two tokens into a function-like macro name, and the ( for that macro's call is separated from it by The motivating bug is real, and the target behavior is correct. I verified against gcc and clang: #define PAR(a, ...) a VA_ARGS
This is legitimate standard-mandated behavior: PREFIX_ ## kind concatenates during argument substitution of DISPATCH, producing However, the fix itself is not a correct implementation of that rescanning — it's a narrow pattern match that silently corrupts output on a Counter-example — just change PAR's body to put a comma between its parameters (equally valid C): #define PAR2(a, ...) a, VA_ARGS
Root cause: the forward scan in DISPATCH2's own replacement list sees PREFIX_ ## kind (→PREFIX_SCALAR), skips the comma, and grabs So the heuristic happens to work for the PR's own test cases only because PAR's body (a VA_ARGS) puts the substituted pieces directly I confirmed the existing test suite (testrunner) still passes on the PR branch, and it compiles clean under -Wall -Wextra -pedantic -Werror — Conclusion: the bug being fixed is real and the desired output is standards-correct, but this specific fix is unsound — it introduces a |
|
Thanks for the thorough review — I've reworked the fix accordingly but before I push it, I want to know your opinion. You were right that the forward scan was unsound. It was walking DISPATCH2's own replacement list and grabbing (VA_ARGS), which really belongs to PAR2's second argument, so it happily produced int x ,. I reproduced your counter-example and confirmed the old branch was silently corrupting it. With AI help, I've dropped the forward scan entirely. expandHashHash now only keeps the case where ( is literally the next token after the pasted name (that one's still needed for things like X##ID(0)). Everything else is left to the rescanner, which is where it belonged. The actual handling now hangs off the existing #682 machinery. After a parameter is substituted, if the output so far ends in a function-like macro name and the substituted parameter starts with (, it builds the call and expands it through the normal Macro::expand path. The important part is where it runs: only on the parameter-substitution branch. That's what makes it match the standard — a ( that came from parameter substitution is genuinely adjacent and binds, but a ( that only appears later because some object-like macro expanded during rescanning does not. Concretely:
all matching gcc/clang now. I checked it against a spread of variants beyond the motivating one — variadic tails, nested indirection, chained rescans, self-referential macros, and pasted names that must stay literal — and they all line up with gcc. Your PAR2 case is now in the regression tests, along with a separate test that pins down the param-supplied-vs-macro-supplied ( boundary, since that's the subtle bit that's easy to regress. Full suite passes, clean under -Wall -Wextra -pedantic, and no leaks under ASan. One small addition to your analysis: this isn't really about ## at all. The same bug shows up with no concatenation — e.g. outer(PREFIX_##k, (VA_ARGS)) behaves the same as a plain two-parameter indirection where a name comes from one arg and the ( from another. That's part of why I moved the logic into the rescan path rather than special-casing the paste site. Let me know if you are agree with this new implementation and I'll push it. |
When the ## operator concatenates two tokens to form a function-like macro name (e.g. PREFIX_ ## kind → PREFIX_SCALAR), simplecpp looked for the argument list '(...)' only at B->next. In PAR-style indirection patterns the '(' is separated from B by a comma or a variadic parameter token:
Because '(' was not found, expansion was aborted and the macro was reported as unknownMacro, causing cppcheck to skip the entire translation unit.
Fix: when B->next is not '(' and we are in the appendTokens context (expandResult==false), walk forward on the same line skipping ',' separators and resolving named parameter tokens via expandArg(). The first '(' found (literally or as the head of an expanded argument) is used as lpar and passed to appendTokens() as before. The forwardScan flag ensures expandToken() is called on the result even when expandResult is false.
The forward scan is restricted to expandResult==false to avoid unintended side-effects in the main expansion loop.