Repository navigation
Fix #15094 SymbolDatabase: Variable::isReference() is true for function pointer returning a reference - #8931
autoantwort wants to merge 7 commits into
Conversation
|
@claude review |
|
Review summary: The fix looks correct for One possible gap (inline comment): pointers to member functions returning a reference ( 🤖 Generated with Claude Code |
|
@claude review |
|
Review summary: The |
|
Test results for commit da724f2 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8931/ 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. |
|
one more claude review I did says: The PR should not be merged as it is: it introduces error-severity false positives on valid code. I built the PR and its merge-base (sequentially, -j4) and compared them; I did not run the testrunner. The false positives All of these are clean on the merge-base and reported with the PR: |
|
Thanks, all of these are valid. They came from the I removed that part. The A self review found two more places that depended on the wrong flags, which are fixed too:
(Written by Claude Code on behalf of @autoantwort.) |
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found no new problems. The Variable::evaluate() change clears only a reference that comes before a function declarator (. In my tests it gave the right flags for every declarator form I tried, including nested function pointers, arrays of function pointers, references to function pointers and pointers to functions returning a reference to an array. On my test files, every warning that changed between base and the PR was a correct new warning or a removed false positive. The full testrunner passes.
Testing
Builds: I built cppcheck and testrunner in pr/, and cppcheck in base/. Both builds succeeded.
Unit tests: ./testrunner TestSymbolDatabase TestAutoVariables TestOther TestConstructors TestClass TestUninitVar all pass. The full ./testrunner -q run reports 5347 tests and 0 failures.
My own inputs (in /work/scratch, run with --enable=all --inconclusive --std=c++17, base compared with PR):
- Members and locals: members
int& (*f)()andstd::string& (*h)(int,int), local function pointers returning references, arrays of them (int& (*arr[2])()),int& (* const p)(), andint& (*&fp)()parameters. Base and PR give the same results, apart from the changes listed below. - Variable flags from
--dump: I checkeddecltype(g())& a,int& (r),int (&arr)[2],int& (*&pr)(),int&& (*&&pr2)()andalignas(8) int& al. The flags are identical on base and PR.int& (r)(x);is simplified by the tokenizer toint & r ( x ), so the new reset inevaluate()is never reached for it. - Changed results, all of them improvements:
int a2(int (&r)()) { return r(); }: base reports aconstParameterReferencefalse positive; the PR does not.Foo(Foo&& (*f)());is no longer treated as a move constructor, so the PR now reportsnoExplicitConstructorfor it, which is correct.C(C& (&f)());likewise gets the correctnoExplicitConstructor.
- Pre-existing, not caused by the PR:
knownPointerToBoolonint&& (&r)() = h;is reported on both base and PR.
I did not run the Python tests in test/cli, because the PR does not touch them.
Automated review of 7e48369 by Claude. The pull request was built and tested locally.
|
Test results for commit 7e48369 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8931/ 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. |
|
This pull request has merge conflicts that must be resolved. |
…on pointer returning a reference For `int& (*f)()` the reference of the return type was taken as reference of the variable, in Variable::isReference() and in its ValueType. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`int& (r)` and `int& UNUSED(r)` lost their reference flag, which gave uninitvar false positives. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…pointer
The type of a call through a function pointer is derived from the ValueType
of the pointer, so resetting the reference caused false positives like
returnTempReference for `int& f(int& (*fp)()) { return fp(); }`.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A reference to a function returning a reference was no longer skipped because isRValueReference() is now false for it. A function reference can't be const-qualified, so it is never reported. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ameter Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7e48369 to
f64500e
Compare
|
I rebased with main. |
|
Test results for commit f64500e (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8931/ 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 new problems. The flag reset in Variable::evaluate() gave the right isReference() and isPointer() for every nested declarator I tried. The new skip in checkConstVariable and the function-reference exclusion in copy/move constructor detection behave as described. Comparing base and PR on my test files, the only change in warnings was a correct new noExplicitConstructor for F(F&& (*&&f)()), which base wrongly took as a move constructor. No false positives were added and none were lost.
Testing
Builds: make -j8 cppcheck testrunner in pr/ and make -j8 cppcheck in base/ both succeeded.
Unit tests:
./testrunner TestSymbolDatabase TestAutoVariables TestOther TestConstructors TestClass: 1153 tests, 0 failures../testrunner -q(full run): 5355 tests, 0 failures.
My own inputs (in /work/scratch, run with --enable=all --inconclusive --std=c++17 on base and PR, outputs compared):
- Members:
std::string& (*f)(int,int),int&& (*g)(),int& (*h[2])(),int& (&r)()andint& (S::*pm)(). - Locals and parameters: locals that are uninitialized, assigned, copied or
static; parametersint& (*const p)(),int (*&p)(),int& (**pp)(), and a template parameterT& (*p)(). - Class checks:
operator=, copy constructors, and setters or getters that use function-pointer members, foruninitMemberVar,operatorEqVarError,uninitStructMemberandfunctionConst. - Constructors:
F(F&& (*&&f)())andF(F (&f)[2]).
Across all of these, the only difference between base and PR was the new noExplicitConstructor for F(F&& (*&&f)()). That warning is correct, because it is a converting constructor and not a move constructor.
Flags from --dump (isReference, base → PR):
| Declarator | Base | PR | Correct |
|---|---|---|---|
int (*(&r)())() |
true | true | yes |
int& (*(*f)())() |
true | false | yes |
int& (*(&f)())() |
true | true | yes |
int (&(*fp)())[3] |
true | false | yes |
int& (* const &f)() |
true | true | yes |
decltype(k())& d |
true | true | yes |
I did not run any Python tests in test/cli, because the PR does not touch them.
Automated review of f64500e by Claude. The pull request was built and tested locally.
https://trac.cppcheck.net/ticket/15094
For a function pointer returning a reference, the reference of the return type was taken as reference of the variable:
Variable::isReference()(andisRValueReference()forfp2) was true for all of them.Variable::evaluate()now drops the reference collected before the(of a function declarator, as it belongs to the return type. A&after the(still makes the variable a reference, soint (*&f)(),int (&f)(),int*& r,int (&a)[2]andint& (r)are unchanged.The
ValueTypeof the function pointer is not changed: the type of a callfp()is derived from it, so its reference has to stay the one of the return type (clearing it causedreturnTempReferenceFPs).Two places relied on the wrong flags and are adjusted:
checkConstVariable: a reference to a function returning a reference (int& (&f)()) was skipped only because it was wrongly flagged as rvalue reference. References to functions are now skipped explicitly (this also fixes the existing FP forint (&f)()).Foo(Foo&& (*f)())was a move constructor. With the fix it is a normal constructor, and parameters that are references to functions (Foo(Foo& (&f)())) no longer make a copy/move constructor either.Found with #8910, where it suppressed
uninitMemberVarNoCtorfor a struct with such a member (codelite in the PR test results). #8910 contains a workaround in CheckClass for this; with this PR merged it can be replaced byvar.isReference()again.This PR was created by Claude Code (an AI coding agent) on behalf of @autoantwort.
🤖 Generated with Claude Code