Repository navigation
Fix #15165 Tokens copied by typedef lose their macro name - #8988
wjakobsson wants to merge 2 commits into
Conversation
|
Test results for commit 84bb93c (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8988/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found no correctness problems. Both changes now carry the macro name over to the copied tokens, and I found no input where the analysis results change.
One gap in test coverage: the new test simplifyTypedefMacroName only exercises the change in TypedefSimplifier::insertTokens (lib/tokenize.cpp). The change in TokenList::copyTokens (lib/tokenlist.cpp) is not tested by anything. If I revert it, every test still passes. That function is used by simplifyUsing, simplifyTypedefCpp, struct splitting and template alias and default-argument copying. Its effect is easy to see, for example with using F = void (*)(A x);, so a test for it would be worth adding.
Testing
Builds: make -j8 cppcheck testrunner succeeded in pr/, and make -j8 cppcheck succeeded in base/.
Unit tests:
./testrunner TestSimplifyTypedef TestSimplifyUsing TestTokenList TestSimplifyTemplate TestTokenizer: 937 tests, 0 failures.- Full
./testrunner -q: 5388 tests, 0 failures.
Which change the new test covers:
- I temporarily reverted only the
lib/tokenlist.cppchange and rebuilt.TestSimplifyTypedefstill passed (0 failures), so thecopyTokenschange is untested. - I temporarily reverted only the
insertTokenschange inlib/tokenize.cpp.simplifyTypedefMacroNamethen failed as expected. - I restored both files and rebuilt afterwards.
The copyTokens path changes behaviour compared with base:
#define A int
using F = void (*)(A x);
void f(F g);cppcheck --debug-normal t3.cpp:
- PR:
void f ( void ( * g@var1 ) ( $int ) ) ; - base:
void f ( void ( * g@var1 ) ( int ) ) ;
Comparing results: I wrote a file that mixes macros with typedef/using arrays, anonymous struct typedefs, a typedef inside a macro, template default arguments from macros, and an unsigned typedef from a macro (d1.cpp). I ran it with --enable=all --inconclusive --check-level=exhaustive, and PR and base gave the same warnings. I also checked the --debug-normal output for typedef function pointers, arrays and struct typedefs. The $ markers now appear on the copied tokens, and I saw nothing wrong.
Automated review of 84bb93c by Claude. The pull request was built and tested locally.
| tok2->varId(tok->varId()); | ||
| tok2->setTokenDebug(tok->getTokenDebug()); | ||
| if (tok->isExpandedMacro()) | ||
| tok2->setMacroName(tok->getMacroName()); |
There was a problem hiding this comment.
AI review
No test covers this change. If I revert it, TestSimplifyTypedef (including the new simplifyTypedefMacroName) and the rest of the testrunner still pass, because the new test only goes through TypedefSimplifier::insertTokens. copyTokens is used by simplifyUsing, simplifyTypedefCpp and the template simplifier, and the effect is visible there. For example, with #define A int / using F = void (*)(A x); / void f(F g);, the PR gives ( $int ) where base gives ( int ). Please add a test for this path, for example in TestSimplifyUsing.
No description provided.