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 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'.
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
New false negative: the isTemplateArg() bail-out that this PR adds to testIfNonZeroExpressionIsPositive() (checkother.cpp:3372) also drops pointerPositive for T* parameters. A T* p is always a pointer, whatever T is, so p >= 0 is never a generic-code false positive. Base reports this case for explicit and for deduced calls. The PR reports neither. An earlier comment covered the same bail-out hiding unsignedPositive for explicit instantiations like pos<unsigned>(u). The pointer case is a different one: the warning is always valid there, and this PR removes it.
The three regressions from the last review (accessMoved on function pointers, the lost return type for calls through auto acb = cb, and the wrong CheckIO message) no longer reproduce at HEAD. The only remaining oddity is that CheckIO now names the argument type 'function *', which is not a C type, but this is cosmetic.
Apart from these, I found no new problems in template deduction. The tests pass.
Testing
Builds
make -j8 cppcheck testrunnerinpr/built.base/did not compile as checked out (settings.platforminlib/vf_analyzers.cpp:361). I applied the same one-line fix that the PR makes and then builtcppcheck.
Tests
./testrunner TestSimplifyTemplate TestSymbolDatabase TestToken TestUnusedVar TestOther TestCondition TestGarbage TestNullPointer TestCmdlineParser TestIO TestLeakAutoVar TestClass TestConstructors: 2632 tests, 0 failed, 159 todos.pytest other_test.py -k "template or showtime": 20 passed, 4 skipped, 1 xfailed.
Lost pointerPositive (t5.cpp)
template<class T> bool f(T* p) { return p >= 0; }
bool g(int* q) { return f<int>(q); }
bool h(int* q) { return f(q); }cppcheck -q --enable=style --template='{line}:{column}: {id} {message}' t5.cpp
- base:
1:43: pointerPositive A pointer can not be negative so it is either pointless or an error to check if it is not.
- PR: no output, also with
--inconclusive. The same happens withif (p >= 0)and a deduced callk(q): base warns, the PR does not.
Rechecks of the last review's findings at HEAD, comparing PR and base on the same files:
std::moveof a function pointer or of a member pointer: noaccessMovedany more, the same as base.printf("%s", acb(1))withauto acb = cb: the warning is reported again.- uninitMemberVar and operatorEqVarError for function-pointer and member-pointer members: the same as base.
printf("%d", cb)now saysthe argument type is 'function *', where base says'signed int *'.
Other probes, no new problems: T&& with an lvalue (not deduced), containers, iterators, unique_ptr::get(), volatile, variadic templates, a non-deduced context (typename T::type), std::vector<T>, T(&)[N], partial explicit arguments, and template vs. non-template overloads (correctly left alone). I also tried unbounded recursion through f(&t) and f(W<T>): neither hung, and the instantiation depth is capped.
Automated review of 436735e by Claude. The pull request was built and tested locally.
| } | ||
|
|
||
| if (const Variable* var = nonZeroExpr->variable()) | ||
| if (var->typeStartToken()->isTemplateArg()) |
There was a problem hiding this comment.
AI review
This bail-out also applies when the variable is declared as a pointer to the template parameter (T* p). That variable is a pointer for every T, so p >= 0 is pointless in generic code too. Base reports pointerPositive for this code, the PR reports nothing, even for an explicit call:
template<class T> bool f(T* p) { return p >= 0; }
bool g(int* q) { return f<int>(q); }
bool h(int* q) { return f(q); }Consider bailing out only when the pointer or unsigned property comes from T itself, for example by skipping the bail-out when var->isPointer() (the declaration has its own *).
|
I added a ticket for the FN. |
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
New false positive: member calls are deduced against free function templates with the same name. deduceFunctionTemplateArguments() does not check whether the call is a member access (obj.f(x) or p->f(x)). The lookup starts at the scope of the call site, so it finds a namespace-level template with the same name and arity. The shape of that template is used to deduce T, and the result is inserted at the member call. When the free template is f(T*) and the member template is f(T), the member gets instantiated with the pointee type instead of the pointer type. This produces a new compareValueOutOfTypeRangeError false positive that base does not report. When the object type is unknown, the call is still rewritten to o.f < int > (a) even though nothing gets instantiated.
I found no other new problems. The earlier review findings were not rechecked.
Testing
Builds
make -j8 cppcheck testrunnerinpr/built.make -j8 cppcheckinbase/built. It no longer needed a fix.
Tests
./testrunner TestSimplifyTemplate TestSymbolDatabase TestToken TestUnusedVar TestOther TestCondition TestGarbage TestNullPointer TestCmdlineParser TestIO TestLeakAutoVar: 2269 tests, 0 failed.pytest other_test.py -k "template or showtime": 20 passed, 4 skipped, 1 xfailed.
Regression sweep: I ran every test/cfg/*.cpp with --enable=all --inconclusive --check-library through the PR and base binaries. The output was identical.
Incremental update vs. full rebuild: I compared the warnings and the --debug-normal output with and without --template-full-rebuild on scratch files. They covered deduction from short, records, pointers, namespaces, members, class template instantiations (W<int>), enum class, and anonymous-namespace and local types. The warnings were identical. Only the variable ids differ, which is expected.
Problem found: wrong deduction for member calls (a9.cpp)
template<class T> void check(T* x) { (void)x; }
struct Range {
template<class T> bool check(T x) { return x != 1000; }
};
bool g(Range& r, unsigned char* p) { return r.check(p); }cppcheck -q --enable=warning,style --template='{line}:{column}: {id} {message}' a9.cpp
- PR:
3:28: functionStatic The member function 'Range::check < unsigned char >' can be static.
3:53: compareValueOutOfTypeRangeError Comparing expression of type 'unsigned char' against value 1000. Condition is always true.
- base: only
functionStatic ... 'Range::check'.
With --debug-normal on the same pattern with int* ip (a4.cpp), the PR outputs p . f<int> ( ip ) and int P :: f<int> ( int x ). T should be int*.
Unknown object type (b1.cpp)
#include "unknown.h"
template<class T> T f(T x) { return 10 / x; }
int g(Unknown& o) { int a = 0; return o.f(a); }With --debug-normal, the PR outputs o . f < int > ( a ). The template arguments are inserted, but no template is instantiated for this call.
Automated review of d65d1b8 by Claude. The pull request was built and tested locally.
| if (instantiationArgs.empty()) | ||
| return ""; | ||
|
|
||
| // a call through a variable (function object, function pointer) is not a template instantiation |
There was a problem hiding this comment.
AI review
Member calls are not excluded here. For obj.f(x) or p->f(x), the lookup below starts at tok->scope(), which is the scope of the calling function. It then finds a namespace-level f template with the same arity. The shape of that unrelated template is used for the deduction, and <...> is inserted at the member call:
template<class T> void check(T* x) { (void)x; }
struct Range { template<class T> bool check(T x) { return x != 1000; } };
bool g(Range& r, unsigned char* p) { return r.check(p); }The PR instantiates Range::check<unsigned char>, but T should be unsigned char*. It then reports a new compareValueOutOfTypeRangeError ("Comparing expression of type 'unsigned char' against value 1000"). Base reports nothing.
When the object type is unknown (Unknown& o; o.f(a)), the call is still rewritten to o . f < int > ( a ).
Suggest bailing out when Token::Match(tok->previous(), ". %name% ("). Alternatively, look the name up in the scope of the object's type.
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
One new false positive: a function declared at block scope does not hide the function template during deduction. With int f(long); declared inside the caller, f(i) should call that function. The PR instead rewrites it to f<int>(i) and reports a zerodiv that base does not report. The guard at templatesimplifier.cpp:1375 only works when the symbol database has linked the call to a non-template Function, and block-scope declarations are never linked. This pattern is rare, so it is a minor issue.
Most earlier findings are fixed at HEAD. These are still open:
signed chardeduces aschar(C36).char16_tandchar32_tdeduce asunsigned shortandunsigned int(C44).- Calls after a binary operator are not deduced (C52).
- A call through a base class is qualified to an instantiation that is never created (C53,
C :: inv<long>). This now also causes a false positive, not just a missed warning:D::hgetsfunctionStaticeven wheninvreads a member, and base does not report that.
Apart from these, I found no new problems.
Earlier comments
29 earlier comments, 4 open
@pfultz2, please fix, resolve or reply to the open comments: #13, #17, #21, #23.
For #21 (false negative), it is enough to create a Trac ticket and link it here. It does not need to be fixed in this PR.
Testing
Builds: make -j8 cppcheck testrunner in pr/ and make -j8 cppcheck in base/ both built.
Unit tests: ./testrunner TestSimplifyTemplate TestSymbolDatabase TestOther TestUnusedVar TestIO TestCondition TestCmdlineParser TestToken TestGarbage ran 1980 tests with 0 failures.
Incremental vs. --template-full-rebuild: I ran --enable=all --inconclusive on test/cfg/std.cpp, test/cfg/stl.cpp and lib/token.cpp. The warnings were identical.
Rechecks of earlier findings: I re-ran the reproducers from comments.md against both binaries and checked the --debug-normal token lists.
- Fixed: member call against a free template (
r.check(p)now givescheck<unsigned char*>), the unknown object type case, recursive calls (f2<long>), nested deduction, the doubledN::C::N::Cqualification,::Sshadowing (it now emitsf<::S>),A<int>base qualification, overloads that differ only inunsigned, theconst T*FP, removal of the generic definition inc3b, theaccessMovedFP, theautofunction-pointer return type, function pointers no longer deduced, and theunsignedPositiveFP. - Still present:
char16_t/char32_tdeduce asf<unsignedshort>/f<unsignedint>, andsigned chardeduces asf<char>. The C53 qualified callC :: inv<long>is still never instantiated.
New FP: block-scope function declaration (h6.cpp)
template<class T> int f(T x) { return 100 / x; }
int g() { int f(long); int i = 0; return f(i); }cppcheck -q --enable=warning,style --template='{line}:{column}: {id} {message}' h6.cpp
- base: no output
- PR:
1:43: zerodiv Division by zero.
--debug-normal shows return f<int> ( i ).
C53 now gives a false positive (p17.cpp)
struct C { int k; template<class U> U inv(U u) const { return k / 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 --enable=style p17.cpp
- base:
5:32: functionStatic ... 'E::inv' - PR: adds
3:10: functionStatic The member function 'D::h' can be static., which is wrong becauseinvreadsk. The cause is the unresolvedC :: inv<long>call that C53 reported.
Automated review of 6db418b by Claude. The pull request was built and tested locally.
| return ""; | ||
| // a call that the symbol database resolved to a real (non template) function must not | ||
| // be hijacked - overload resolution against non templates is not implemented here | ||
| if (tok->function() && !tok->function()->templateDef) |
There was a problem hiding this comment.
AI review
This guard only works when the symbol database has linked the call to a Function. A function declared at block scope is never linked, so it does not hide the template, and the call gets deduced anyway:
template<class T> int f(T x) { return 100 / x; }
int g() { int f(long); int i = 0; return f(i); }The PR rewrites the call to f<int>(i) and reports zerodiv at 1:43. Base reports nothing. C++ name lookup finds the local f(long) first and stops there, so the template is not a candidate. Consider bailing out when the enclosing block scopes declare a non-template %name% ( with the same name.
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.