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
比较
… 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
比较
|
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 |
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.