Repository navigation
Fix #15165 Tokens copied by typedef lose their macro name - #8988
wjakobsson wants to merge 3 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
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.
84bb93c to
1011c11
Compare
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
New false negative: the copied macro names make some checks skip code that only uses a typedef. Checks that ignore macro-expanded code now also ignore code that uses a typedef whose definition came from a macro. I reproduced this with duplicateBranch. With #define INT int / typedef INT T;, two identical branches that both use T are reported by base but not by the PR. CheckOther::checkDuplicateBranch stops when any token in the branches isExpandedMacro(). That rule exists because different macros can expand to the same code, but here both branches spell the same typedef T, so they really are duplicates. Other checks that skip macro tokens could lose warnings the same way: isConstStatement, compareTokenFlags/isSameExpression, checkRedundantPointerOp, and unusedStructMember via classDef. Maintainers should decide whether every typedef use site should count as "expanded from macro X" for these checks, or whether only the debug/dump output needs the macro name. Apart from this I found no new problems. The test-coverage gap for copyTokens that was raised earlier is now covered by simplifyUsingMacroName.
Testing
Build: make -j8 cppcheck testrunner in pr/ and make -j8 cppcheck in base/ both succeeded.
Unit tests: ./testrunner TestSimplifyTypedef TestSimplifyUsing TestOther TestTokenizer ran with 0 failures, including the new simplifyTypedefMacroName and simplifyUsingMacroName.
Regression (false negative), db.c:
#define INT int
typedef INT T;
void g(int);
void f(int c) {
if (c) {
T x = 1;
g(x);
} else {
T x = 1;
g(x);
}
}cppcheck -q --enable=style --inconclusive db.c:
- base:
db.c:5:5: style: inconclusive: Found duplicate branches for 'if' and 'else'. [duplicateBranch] - PR: no output.
The same thing happens with casts, r = g((T)c); in both branches: base warns and the PR does not.
Other checks:
(T)a;withTtypedef'd from a macro still givesconstStatementin both versions.// cppcheck-suppress-macroon theINTmacro does not suppress warnings on the lines whereTis used, in either version, so suppression behaviour has not changed.
Automated review of 1011c11 by Claude. The pull request was built and tested locally.
| to->isSimplifiedTypedef(true); | ||
| to->isExternC(from->isExternC()); | ||
| if (from->isExpandedMacro()) | ||
| to->setMacroName(from->getMacroName()); |
There was a problem hiding this comment.
AI review
Every token copied from the typedef body now counts as isExpandedMacro() at each place the typedef is used. Checks that skip macro code therefore skip this code too. Example: #define INT int / typedef INT T; / if (c) { T x = 1; g(x); } else { T x = 1; g(x); }. --enable=style --inconclusive gives duplicateBranch on base and nothing with this PR, because checkDuplicateBranch stops on any expanded-macro token in the branches. Both branches use the same typedef, so the 'different macros, same expansion' reasoning doesn't apply here. The same could affect isSameExpression/compareTokenFlags, isConstStatement, checkRedundantPointerOp and similar checks. Should the macro name only be kept for debug/dump output, or should these checks treat isSimplifiedTypedef() tokens differently?
No description provided.