Repository navigation
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
So I ran some numbers comparing the time with valueflow disabled:
It seems to be about ~20-30% slower, which is much better than what it was before at ~80% slower doing full rebuilds. Partial rebuilds seem to really help a lot with this. Obviously since we are doing more instantiations than before it is going to be slower even as there may be room to improve this further. I think this is an acceptable slowdown and in the end I think this will still be faster than the approach in #8688 as we will have to always compute the valuetypes twice since they are seperate components. So I think this approach is ultimately better. @danmar @chrchr-github What are you thoughts on this approach? I can work on cleaning up this PR and fixing the CI failures if you think this is the better way to go. |
|
Test results for commit 71c2a23 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8720/ 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 two new false-positive problems. In both, deduction now triggers older expandTemplate/removal behaviour much more often. This is the same pattern as the doubled N::C::N::C qualification that was reported earlier.
- New
unusedStructMemberFP for class templates that pass*thisto a function template.f(*this)inside an uninstantiated class template is (correctly) not deduced. But once the deduced instantiations elsewhere have been made, the generictemplate<class T> ... get(const T&)definition is removed. The only use of the member in the token list then disappears. Base reports nothing for this code. Base does show the same FP if the other call is written explicitly asget<Bag>(b). - Recursive calls with different argument types are bound to the outer instantiation. Inside an instantiated body,
expandTemplaterenames every call of the template's own name to the current instantiation. With deduction,f2(i)withint itherefore producesf2<int>(l)for along largument, andf(t)producesf<int>("x")for a string literal argument. I could not turn this into a warning in the time I had, but the token list and the AST are wrong. Without the PR, the template is never instantiated in these cases.
I found nothing else beyond what comments.md already covers. Nested deduction in bodies that type deduction created is still missed (f(&t) stays undeduced in my b4 test), but that has already been reported.
Testing
I built cppcheck and testrunner in pr/, and cppcheck in base/.
./testrunner TestSimplifyTemplate TestOther TestCondition TestGarbage TestToken TestCmdlineParser TestSymbolDatabase TestTokenizer TestVarID: 2318 tests, 0 failed (74 todos).pytest other_test.py -k "template or debug": 21 passed, 1 skipped, 2 xfailed.- Other cases I tried: a template parameter that shadows a class name, a class template member passed to a function template,
::f(i)next to a memberf, ADL, unbounded recursion through class templates (f(T)callingf(W<T>)),set(this)in a const member, lambdas, andnullptr/0/NULLforT*. None crashed or hung. Apart from the two findings below, the results were correct or not new.
New FP: unusedStructMember (c3b.cpp)
template<class T> int get(const T& t) { return t.count; }
template<class U> struct Box {
U item;
int count;
int size() const { return get(*this); }
};
struct Bag { int count; };
int total(const Bag& b) { return get(b); }cppcheck -q --enable=warning,style c3b.cpp
- base: only
uninitMemberVarNoCtorforBox::count. - PR: additionally
c3b.cpp:4:9: style: struct member 'Box::count' is never used. [unusedStructMember]
Base gives the same FP when the last call is written as get<Bag>(b), so the removal of the generic definition is older than this PR. The PR now triggers it for plain calls.
Recursive call bound to the wrong instantiation (b4.cpp)
template<class T> int f(T t) { return sizeof(t); }
template<class T> int f2(T t) { long l = 0; return f2(l) + f(&t); }
int g(int i) { return f2(i); }cppcheck -q --debug-normal b4.cpp (PR):
2: int f2<int> ( int t@var6 ) { long l@var7 ; l@var7 = 0 ; return f2<int> (@exprUNIQUE l@var7 ) +@exprUNIQUE f (@exprUNIQUE &@exprUNIQUE t@var6 ) ; }
The call f2(l) should instantiate f2<long>. Similarly, template<class T> int f(T t){ if (t == 0) return f("x"); return 1; } with f(i) gives f<int> ( "x" ). Base leaves these templates alone. I could not get a wrong warning out of this with the time I had.
Automated review of 06e5836 by Claude. The pull request was built and tested locally.
| rememberNewTokens(beforeQualification ? beforeQualification->next() : mTokenList.front(), tok->previous()); | ||
| } | ||
|
|
||
| // insert the deduced template arguments: "f ( x )" => "f < int > ( x )" |
There was a problem hiding this comment.
AI review
Once a deduced instantiation has been made, the generic definition is removed. A use of the template in an uninstantiated class template (get(*this), which is correctly not deduced) then disappears from the token list, and that gives a new FP:
template<class T> int get(const T& t) { return t.count; }
template<class U> struct Box { U item; int count; int size() const { return get(*this); } };
struct Bag { int count; };
int total(const Bag& b) { return get(b); }With this PR: style: struct member 'Box::count' is never used. [unusedStructMember]. Base gives no such warning. Base shows the same FP for an explicit get<Bag>(b), so the cause is older, but deduction now makes it happen for ordinary calls.
| insertDeducedType(tok, deducedTypes[i - 1]); | ||
| if (i > 1) | ||
| tok->insertToken(","); | ||
| } |
There was a problem hiding this comment.
AI review
When expandTemplate copies the body of a deduced instantiation, it renames every call of the template's own name to the current instantiation, whatever the argument types are. template<class T> int f2(T t) { long l = 0; return f2(l); } called as f2(i) with int i gives f2<int> ( l ) in the f2<int> body. The same happens for f("x") inside f<int>, which becomes f<int> ( "x" ). Base never instantiated these templates, so the PR now makes this common. Those recursive calls should be left for deduction, or at least not be renamed when they have type information that disagrees.
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
New false positive: overloads that differ only in unsigned, long or signed on a non-template parameter are treated as one template, so the deduced call is bound to the wrong overload. DeductionCandidate::signature uses only shape.typeTok->str() for a concrete parameter type. After simplification, int, unsigned int, long and long long, and char and signed char, are the same token string (int, long, char) with different isUnsigned/isLong/isSigned flags. Take template<class T> f(T, int) and template<class T> f(T, unsigned int). Both get the same signature, so the overload bail-out at line 1405 sees them as a forward declaration plus its definition and does not bail out. f(x, y) with an int y then becomes f<int>, and the unsigned int overload is expanded for it. I reproduced a zerodiv FP that base does not give. Base gives the same FP for an explicit f<int>(x, y), so the expansion behaviour is older. But the overload check this PR added is meant to prevent exactly this for deduced calls.
I found no other new problems. The remaining issues I saw (signed char/char16_t merging, recursive calls bound to the outer instantiation, generic definitions removed) are already in comments.md.
Testing
I built cppcheck and testrunner in pr/, and cppcheck in base/.
./testrunner TestSimplifyTemplate TestOther TestCondition TestGarbage TestToken TestCmdlineParser TestUnusedVar: 1503 tests, 0 failed (102 todos).
Wrong overload chosen (o3.cpp):
template<class T> T f(T a, int b) { return b < 0 ? a : a + b; }
template<class T> T f(T a, unsigned int b) { return a / b; }
int g(int x) { int y = 0; return f(x, y); }cppcheck -q --enable=warning,style o3.cpp
- base: no output
- PR (the same with
--template-full-rebuild):
o3.cpp:2:55: error: Division by zero. [zerodiv]
template<class T> T f(T a, unsigned int b) { return a / b; }
o3.cpp:3:39: note: Calling function 'f < int >', 2nd argument 'y' value is 0
In C++ the call goes to the int overload, which does not divide. --debug-normal shows int f<int> ( int a , unsigned int b ) { return a / b ; } as the only instantiation.
The same thing happens with char and signed char (o5.cpp: f(T a, char b) and f(T a, signed char b) { return a / b; }, called with char y = 0): the PR gives zerodiv and base gives nothing. With long and long long (o2.cpp), --debug-normal shows the long long overload instantiated for the call.
Base gives the same zerodiv for an explicit f<int>(x, y) (o4.cpp), so the expansion bug itself is older. Deduction now reaches it for plain calls.
Automated review of 255e8a3 by Claude. The pull request was built and tested locally.
| if (shape.templateParameterIndex >= 0) | ||
| parsedCandidate.signature += " $" + std::to_string(shape.templateParameterIndex); | ||
| else | ||
| parsedCandidate.signature += " " + shape.typeTok->str(); |
There was a problem hiding this comment.
AI review
For a concrete parameter type, the signature contains only typeTok->str(). After simplification, unsigned int/int, long long/long and signed char/char have the same string and differ only in isUnsigned()/isLong()/isSigned(). So template<class T> T f(T a, int b) and template<class T> T f(T a, unsigned int b) get the same signature, and the check at line 1405 takes them for a forward declaration and its definition. It does not bail out. int y = 0; f(x, y); then instantiates the unsigned int overload, and return a / b; in that overload gives a new zerodiv FP (base: no output). Please add these flags to the signature, e.g. (typeTok->isUnsigned() ? "unsigned " : "") + (typeTok->isLong() ? "long " : "") and the same for isSigned().
|
Test results for commit 255e8a3 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8720/ 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 new problems that cause a wrong warning. All the false positives I could reproduce are either already in comments.md or happen in base as well.
One new gap (a missed warning, not a wrong one): a call is only deduced when the token before it is one that isTemplateInstantion() accepts. A call that comes after +, -, ==, &&, ? and similar operators is never deduced, so return twice(l) + twice(k); only instantiates the first call. The cause is older code: base doesn't instantiate an explicit 1 + twice<int>(k) either. With this PR, though, it can make deduction look like it works in one statement and fail in a similar one. For example, zerodiv is reported for return inv(z); but not for return 1 + inv(z);. It could be fixed in this PR or tracked in a ticket. The check is outside the lines this PR changes, so I left no inline comment.
Incremental updates and --template-full-rebuild gave the same results on everything I tried.
Testing
Build: make -j8 cppcheck testrunner in pr/ and make -j8 cppcheck in base/ both built.
Unit and Python tests
./testrunner TestSimplifyTemplate TestOther TestCondition TestGarbage TestToken TestCmdlineParser TestUnusedVar TestSymbolDatabase: 1932 tests, 0 failed, 116 todos.pytest other_test.py -k "template or debug": 21 passed, 1 skipped, 2 xfailed.pytest other_test.py -k showtime: 15 passed, 4 skipped. This covers the changedexp_resvalues.
Incremental update vs --template-full-rebuild
- I ran
--enable=all --inconclusiveboth ways on all 13test/cfg/*.cppfiles and onlib/token.cpp,lib/library.cppandlib/mathlib.cpp. The output was identical. - For two template-heavy files of my own, I compared the
--debug-normaltoken lists with the varids/exprids removed:i1.cpp: templates in a namespace, a class template whose member calls a function template, and a member template next to access specifiers. Only the varid numbers differed.j1.cpp: a class template instantiated inside a deduced body, a private member template, andT*deduction. The lists were identical.
Missed deduction after a binary operator
// i3.cpp
template<class T> T twice(T x) { return x + x; }
int h(int k) { long l = 0; return twice(k) + twice(l); }cppcheck -q --debug-normal i3.cpp (PR):
4: return twice<int> (@exprUNIQUE k@var2 ) +@exprUNIQUE twice (@exprUNIQUE l@var3 ) ;
twice(l) stays generic. When the two calls are in separate statements (i7.cpp) or separate functions (i6.cpp), both are deduced.
// i9.cpp
template<class T> T inv(T x) { return 100 / x; }
int h(int k) { int z = 0; return 1 + inv(z); }
int h2(int k) { int z = 0; return inv(z); }With the PR, zerodiv is reported only through h2. --debug-normal shows return 1 + inv ( z ) left alone in h. Base doesn't instantiate an explicit 1 + twice<int>(k) either (i8.cpp), so the limit is older than this PR.
Other probes: none of these crashed or gave a new FP compared with base.
- Member calls
s.f(y),this->f(y)and(*p).f(y)when a free template has the same name, plus a member template reached through a derived class. - A qualified
N::f(y), lambdas,?:, and mixedint/longarguments form(T, T). const int*passed toT*.- Deducing
W<int>andW<int>::In. Here the instantiated parameter is writtenW < int > :: Ininstead ofW<int> :: In, but I could not find any effect on the output.
Automated review of bc64680 by Claude. The pull request was built and tested locally.
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found one new problem: when a member template is found in a base class that is a class template instantiation, the base-class qualification inserted at the call site is broken up into separate tokens. In struct B : A<int>, the call inv(z) becomes A < int > :: inv<long> ( z ), not A<int> :: inv<long> ( z ). The scope's className is "A < int >", and splitString(baseClassQualification, ' ') turns it into four tokens. The instantiation A<int> :: inv<long> is created, but the call is never linked to it: there is no Function and no return ValueType on the call. Main does report a null-pointer bug in this setup (nullPointer and ctunullpointer) that the PR no longer reports.
The second finding is a missed warning, and its cause is older. If a second class has a member template with the same name, a deduced call through a non-template base gets qualified (C :: inv<long>), but C::inv<long> is never instantiated. Main behaves the same with an explicit C::inv<long>(z), but deduction now hits this with plain calls. As in the earlier reports in comments.md, older expansion behaviour is now reached far more often.
Apart from these two, I found no new problems.
Testing
I built cppcheck and testrunner in pr/, and cppcheck in base/.
./testrunner TestSimplifyTemplate TestOther TestCondition TestGarbage TestToken TestCmdlineParser TestUnusedVar TestSymbolDatabase TestTokenizer TestVarID: 2550 tests, 0 failed, 121 todos.pytest other_test.py -k "template or debug or showtime": 36 passed, 5 skipped, 2 xfailed.
Lost warning: member template in a class-template base (q2.cpp)
template<class T> struct A {
template<class U> void put(U* p) const { *p = 1; }
};
struct B : A<int> {
void h() const { long* p = nullptr; put(p); }
};cppcheck -q q2.cpp
- base:
q2.cpp:2:47: warning: Possible null pointer dereference: p [nullPointer]
q2.cpp:5:45: note: Calling function 'put', 1st argument 'p' value is 0
q2.cpp:2:47: error: Null pointer dereference: p [ctunullpointer]
- PR: no output.
- With a non-template base (
struct A {...}; struct B : A, inq3.cpp), the PR still reportsnullPointerwithCalling function 'put < long >'.
The token list explains it (q1.cpp: the same code, but with template<class U> U inv(U u) const { return 100 / u; } and long z = 0; return inv(z);). cppcheck -q --debug-normal q1.cpp, PR:
5: long h ( ) const { long z@var1 ; z@var1 = 0 ; return A < int > ::@exprUNIQUE inv<long> (@exprUNIQUE z@var1 ) ; }
The AST of that call has no f: on inv < long > and no type on (. With the explicit A<int>::inv<long>(z) (q1x.cpp), the token is A<int>, the call has f:0x..., and ( has type 'signed long'.
Qualified call to an instantiation that is never made (p15.cpp)
struct C { template<class U> U inv(U u) const { return 100 / u; } };
struct D : C {
long h() const { long z = 0; return inv(z); }
};
struct E { template<class U> U inv(U u) const { return 100 / u; }
long h() const { long z = 0; return inv(z); } };cppcheck -q --debug-normal p15.cpp, PR:
1: struct C { template < class U > U inv ( U u@var1 ) const { return 100 /@exprUNIQUE u@var1 ; } } ;
3: ... return C ::@exprUNIQUE inv<long> (@exprUNIQUE z@var2 ) ; }
5: struct E { long inv<long> ( long u@var5 ) const ;
No C :: inv<long> exists, so zerodiv is reported only for E, not for D. Without E (p12.cpp), D is instantiated and zerodiv is reported. --template-full-rebuild gives the same result. Base gives the same result with an explicit C::inv<long>(z) (p15x.cpp), so the cause is older.
Other probes: none of these gave a new wrong warning compared with base.
size_t/typedef arguments,W<int>arguments, aT*with an array, aconst T&withint*(correctlyint * const &), float literals,nullptr, and promotedunsigned char.- Member templates in plain classes and in namespaces, member templates of class-template instantiations, and a call in a constructor initializer list.
- Incremental and
--template-full-rebuildgave the same warnings. Onp3.cppthe only differences were the order of the messages and the function name in them (C::helper < int >). - A generated file with 300 class/function template pairs and 900 deduced calls ran in 2.2 s with the PR. The same file with explicit template arguments took 1.9 s with base, so there is no performance problem.
Automated review of cf53504 by Claude. The pull request was built and tested locally.
| // instantiation is matched with that declaration: "btf ( x )" => "Base :: btf ( x )" | ||
| if (!baseClassQualification.empty()) { | ||
| Token* const beforeQualification = tok->previous(); | ||
| for (const std::string& part : splitString(baseClassQualification, ' ')) |
There was a problem hiding this comment.
AI review
When the base class is a class template instantiation, baseClassQualification comes from the scope's className, which is "A < int >". Splitting it at spaces inserts the four tokens A < int >, not the single token A<int> that the instantiation uses:
template<class T> struct A { template<class U> void put(U* p) const { *p = 1; } };
struct B : A<int> { void h() const { long* p = nullptr; put(p); } };The PR turns the call into A < int > :: put<long> ( p ). The symbol database never links that call to A<int> :: put<long>: the call has no Function and no return ValueType. Base reports nullPointer and ctunullpointer for this code, the PR reports nothing. With the explicit A<int>::put<long>(p) the call is linked. Please insert the class-template part as one token (as the tokenizer would write it), or don't deduce through such bases.
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found one new problem: function pointers and pointers to members are deduced as plain object pointers. For int (*cb)(int) and for int S::*pm, the argument's ValueType is INT with pointer == 1. argumentToDeducedType cannot tell these apart from int *, so invoke(cb, 3) becomes invoke<int*>(int * cb, ...) and get(s, pm) becomes get<int*>(const S & s, int * pm). The body is then analysed with a data pointer where the code calls the pointer or uses .*. A function-pointer call and a real int * call would also share one f<int*> instantiation. I could not get a wrong warning out of this in the time I had, but the token list is wrong, and base never instantiates these templates.
Apart from this I found no new problems. Nested deduction (g(t) → f(t) inside a deduced body) now works. Anonymous, local and anonymous-namespace types are correctly not deduced, and calls in uninstantiated templates whose parameter name matches a real class (struct T + template<class T>) are left alone.
Testing
I built cppcheck and testrunner in pr/, and cppcheck in base/. Both builds worked.
./testrunner TestSimplifyTemplate TestOther TestCondition TestGarbage TestToken TestCmdlineParser TestUnusedVar TestSymbolDatabase TestNullPointer: 2077 tests, 0 failed, 126 todos.pytest other_test.py -k "template or debug": 21 passed, 1 skipped, 2 xfailed.
Function pointer / pointer to member deduced as int* (d2.cpp, d3.cpp):
// d2.cpp
template<class T> int invoke(T cb, int x) { return cb(x); }
int twice(int x) { return 2 * x; }
int h() { int (*cb)(int) = twice; return invoke(cb, 3); }
// d3.cpp
struct S { int a; int b; };
template<class T> int get(const S& s, T pm) { return s.*pm; }
int h(const S& s) { int S::*pm = &S::b; return get(s, pm); }cppcheck -q --debug-normal d2.cpp / d3.cpp (PR; --template-full-rebuild gives the same):
1: int invoke<int*> ( int * cb@var7 , int x@var8 ) { return cb@var7 (@exprUNIQUE x@var8 ) ; }
2: int get<int*> ( const S & s@var9 , int * pm@var10 ) { return s@var9 .@exprUNIQUE *@exprUNIQUE pm@var10 ; }
In d1.cpp, f(p) with int (*p)(int) and f(pm) with int M::*pm both map to the same f<int*>. I tried a few bodies (d4.cpp, d5.cpp: null checks, s.*pm = 0, p[1]) with --enable=all --inconclusive. The warnings were the same as base, so I don't have a reproduced FP for this.
Other probes, no new problems:
- A call in a deduced body (
g<int>→f<int>) is deduced. - Global initializers, default member initializers, constructor initializer lists and lambdas.
- Class template instantiation in the same round (full rebuild), with the same warnings both ways.
- Typedef'd anonymous struct (deduced as
S), an unnamed struct variable, a local struct, an anonymous namespace type, unions, lambdas,volatile, function names and&M::m(not deduced, which is correct). - A generic template whose parameter name matches a real class (
struct T+template<class T>, also in a class template), which is not deduced.
Automated review of eecfc81 by Claude. The pull request was built and tested locally.
| const ValueType* const vt = arg->valueType(); | ||
| if (!vt) | ||
| return DeducedType(); | ||
| if (vt->bits > 0 || vt->volatileness != 0) |
There was a problem hiding this comment.
AI review
Function pointers and pointers to members get through this filter. For int (*cb)(int) and int S::*pm the ValueType is INT with pointer == 1, so both are deduced as int *:
template<class T> int invoke(T cb, int x) { return cb(x); }
int twice(int x) { return 2 * x; }
int h() { int (*cb)(int) = twice; return invoke(cb, 3); }This gives int invoke<int*> ( int * cb , int x ) { return cb ( x ) ; }. get(s, pm) with int S::*pm gives get<int*>(const S & s, int * pm) { return s . * pm ; }. These calls also share an instantiation with real int * calls. Base does not instantiate these templates. Please bail out when the argument is a function pointer or a member pointer. For example, check the argument variable's declaration (( * name ) (, :: *), or skip pointer arguments that are not plain variables or expressions with an object pointer type.
I have the feeling we have done this before.. I can accept it if you do it on purpose here. We can have the goal to fix this later. |
|
I added support for function pointers with https://trac.cppcheck.net/ticket/15140 |
|
This pull request has merge conflicts that must be resolved. |
|
@danmar Any more reviews? |
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found three new regressions. All three come from the new ValueType::FUNCTION/MEMBER_POINTER types, not from template deduction. Before this PR, int (*cb)(int) was typed signed int *. It is now typed function *. The new enum values sit between ITERATOR and VOID, and several places in the code order-compare ValueType::Type, so they now see these types differently:
- New
accessMovedfalse positive.isMoveOrForward(valueflow.cpp:3169) only skips types>= VOID. Function pointers and pointers to members are belowVOIDnow, sostd::move(cb)counts as a real move. Any later use ofcbis then reported, even though moving a pointer copies it. - Calls through an
autovariable that holds a function pointer lose their return type (symboldatabase.cpp:7519). The new branch returns early even whenfunctionDeclarator()finds no( * name )declarator. That is the case forauto acb = cb;. Base givesacb(1)the typeint, the PR gives it no type, andinvalidPrintfArgTypewarnings are lost. - CheckIO messages are now wrong. checkio.cpp:1557 only uses the AST type for
type >= BOOL. Function pointers now go through the old token-based path, and the message reads "requires 'int' but the argument type is 'int'" (or'auto').
The misra addon output on misra-test.c/.cpp is unchanged. I found no new problems in the template deduction part beyond what comments.md already covers.
Testing
Builds
make -j8 cppcheck testrunnerinpr/built.base/did not compile as checked out:lib/vf_analyzers.cpp:361hassettings.platform, and the PR fixes this tosettings().platform. I applied the same one-line fix to base so I could compare.
Tests
./testrunner TestSimplifyTemplate TestSymbolDatabase TestToken TestUnusedVar TestOther TestCondition TestGarbage TestNullPointer TestCmdlineParser TestTokenizer TestVarID: 2701 tests, 0 failed, 131 todos.pytest other_test.py -k "template or showtime": 20 passed, 4 skipped, 1 xfailed.- misra addon on
addons/test/misra/misra-test.candmisra-test.cpp: the findings are the same for base and PR.
Probes with no change from base: nullPointer checks on function pointers and member pointers, uninitMemberVar and operatorEqVarError for members of these types, casts, comparisons, sizeof, and struct members that are function pointers (s.cs(1)).
All the commands below use --template='{file}:{line}:{column}: {id} {message}'.
1. accessMoved FP (d1.cpp)
#include <utility>
int use(int (*cb)(int)) {
int (*c)(int) = std::move(cb);
return cb(1) + c(2);
}
struct S { int a; };
int use2(S& s, int S::*pm) {
int S::*q = std::move(pm);
return s.*pm + s.*q;
}
void sink(int (*)(int));
int use3(int (*cb)(int)) {
sink(std::move(cb));
return cb(3);
}cppcheck -q --enable=all --inconclusive d1.cpp: base reports none of these. The PR adds:
d1.cpp:4:12: accessMoved Access of moved variable 'cb'.
d1.cpp:9:15: accessMoved Access of moved variable 'pm'.
d1.cpp:14:12: accessMoved Access of moved variable 'cb'.
2. Return type lost for a call through auto (f1.cpp)
#include <cstdio>
void p(int (*cb)(int), char* (*cs)(int)) {
auto acb = cb;
auto acs = cs;
printf("%s\n", acb(1));
printf("%d\n", acs(1));
printf("%s\n", cb(1));
}- base:
f1.cpp:5:5: invalidPrintfArgType_s %s in format string (no. 1) requires 'char *' but the argument type is 'signed int'.
f1.cpp:6:5: invalidPrintfArgType_sint %d in format string (no. 1) requires 'int' but the argument type is 'char *'.
f1.cpp:7:5: invalidPrintfArgType_s ...
- PR: only the line 7 warning remains.
3. Wrong CheckIO message (c1.cpp, excerpt)
void p(int (*cb)(int), S s, int P::*pm) { printf("%d %d\n", s.cb, pm); printf("%f %u\n", cb, cb); auto acb = cb; printf("%d\n", acb); }- base:
... requires 'int' but the argument type is 'signed int *'. - PR:
... requires 'int' but the argument type is 'int'., and foracb:... the argument type is 'auto'.
Automated review of e521191 by Claude. The pull request was built and tested locally.
| SMART_POINTER, | ||
| CONTAINER, | ||
| ITERATOR, | ||
| FUNCTION, ///< a function; a pointer to it ("int (*)(int)") is counted in pointer |
There was a problem hiding this comment.
AI review
These two values sit between ITERATOR and VOID, and several places order-compare ValueType::Type. For example, isMoveOrForward() in valueflow.cpp:3169 only skips types >= VOID. Function pointers and pointers to members used to be INT and were skipped there. Now std::move(cb) is treated as a real move and gives a new FP:
#include <utility>
int use(int (*cb)(int)) { int (*c)(int) = std::move(cb); return cb(1) + c(2); }accessMoved: Access of moved variable 'cb'. The same happens for int S::*pm. Base reports nothing.
The other ordered comparisons need the same review: checkio.cpp:1557 >= BOOL, valueflow.cpp:4109 < VOID, astutils.cpp:3717 <= VOID, checkclass.cpp:92 >= CHAR, checkother.cpp:574, and isPrimitive(). Alternatively, place the new values after VOID.
| ValueType returnType; | ||
| if (declarator && parsedecl(var->typeStartToken(), &returnType, mDefaultSignedness, mSettings, declarator)) | ||
| setValueType(parent, returnType); | ||
| return; |
There was a problem hiding this comment.
AI review
This returns even when functionDeclarator() finds no ( * name ) declarator, for example for auto acb = cb;. The call acb(1) then gets no ValueType at all. Base typed it int through the generic pointer - 1 branch below. Example:
void p(int (*cb)(int), char* (*cs)(int)) { auto acb = cb; auto acs = cs; printf("%s\n", acb(1)); printf("%d\n", acs(1)); }Base reports invalidPrintfArgType_s and invalidPrintfArgType_sint. The PR reports nothing. Consider returning only when the return type was set, or deriving the return type from the auto variable's initializer.
| CONTAINER, | ||
| ITERATOR, | ||
| FUNCTION, ///< a function; a pointer to it ("int (*)(int)") is counted in pointer | ||
| MEMBER_POINTER, ///< a pointer to a data member ("int S::*"), counted in pointer |
There was a problem hiding this comment.
AI review
CheckIO uses the AST type only for valuetype->type >= ValueType::Type::BOOL (checkio.cpp:1557). Function pointers now fall back to the old token-based code, so the messages are wrong: printf("%d", cb) with int (*cb)(int) gives "%d in format string (no. 1) requires 'int' but the argument type is 'int'." Base said 'signed int *'. For an auto acb = cb; argument, the PR says the type is 'auto'.
This creates a loop where we run the TemplateSimplifier -> varids/links/AST/SymbolDatabase/ValueTypes, so on the second round of
TemplateSimplifierwe can use type information from theSymbolDatabase. Not all the passes need to run withTemplateSimpliferso there is aSymbolDatabase::finalize().On a naive run, it would delete all the AST/SymbolDatabase information, but this can be kind of slow. So I added some incremental updates by tracking where new tokens are added(plus the function bodies around changed call sites), and then refactoring functions to work on token ranges, so there is now
TokenList::createAst(start, end)and in SymbolDatabase:addSymbolsForNewTokenRanges(),updateFunctionAndVariablePointers(), a range-limitedsetValueTypeInTokenList()andfindAllScopes(const Token* startToken, const Token* endToken, Scope* startScope). Also thecreateSymbolDatabaseFindAllScopesinternals were split into reusable per-scope/per-function helpers to support this.Now this only does incremental updates on function instantiation because its much simpler to do as it only adds a
Function, its scope and locals(and the only stale references in old code are the renamed call sites, whichupdateFunctionAndVariablePointers()re-resolves by name). Instantiating a class introduces new functions and types that could be resolved in other parts of the code. So for this case it does a full rebuild.Also the template alias simplifications requires a full build as well as it restructures existing tokens throughout the list, not just in new ranges. So the "unchanged tokens keep valid info" premise of the incremental update no longer holds.
We can probably address these in the future but will require a larger refactor. I also added a
--template-full-rebuildso we can debug any issue with the incremental updates.