Fix #15094 SymbolDatabase: Variable::isReference() is true for function pointer returning a reference

[autoantwort]

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:

struct S {
    std::string& (*f)(int, int);
    int& (*g[2])();
};
void foo(int& (*fp)(), int&& (*fp2)());

Variable::isReference() (and isRValueReference() for fp2) 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, so int (*&f)(), int (&f)(), int*& r, int (&a)[2] and int& (r) are unchanged.

The ValueType of the function pointer is not changed: the type of a call fp() is derived from it, so its reference has to stay the one of the return type (clearing it caused returnTempReference FPs).

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 for int (&f)()).
• Copy/move constructor detection: 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 uninitMemberVarNoCtor for 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 by var.isReference() again.

This PR was created by Claude Code (an AI coding agent) on behalf of @autoantwort.

🤖 Generated with Claude Code

[dmcppcheck]

@claude review

[claude]

Review summary: The fix looks correct for int& (*f)(), arrays of function pointers, and int (*&f)(). Redundant declarator parentheses such as char &(b); are already removed by the tokenizer (removeParentheses22), so the new ( reset in Variable::evaluate() shouldn't misclassify real references.

One possible gap (inline comment): pointers to member functions returning a reference (int& (S::*pm)()). evaluate() clears the flag, but parsedecl() breaks before the reset because it only handles ( *, so ValueType::reference may still be LValue. A test for that case would be good.

🤖 Generated with Claude Code

[dmcppcheck]

@claude review

[claude]

Review summary: The Variable::evaluate() change looks right. Only a reference that comes before the declarator ( is dropped, so int (*&f)(), int (&f)(), int& (r) and UNUSED(r) keep their reference. I left one inline comment: in parsedecl() the reset only applies to ( *. It looks like pointer-to-member declarators (int& (S::*pm)()) leave the loop before the reset, so their ValueType would still say LValue reference. Please check that the new pm test really passes. I reviewed the code by reading it only; I didn't build it or run the tests.

[dmcppcheck]

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/
packages: 100
Differing warnings: 0 (+0 -0)
Timing: -0.2% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8931/2026-10-09_20-01-23-da724f2e5700_timing.html)

_{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.}

[danmar]

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:

  int& use1(int& (*fp)()) { return fp(); }                 // returnTempReference
  void use3(int& (*fp)()) { int& r = fp(); r = 2; }        // danglingTempReference
  struct D {
      int& call() { return f(); }                          // returnTempReference
      int& (*f)();
  };
  const int& use4(const int& (*fp)(int)) { return fp(1); } // returnTempReference
  int& (*g)();
  int& use5() { return g(); }                              // returnTempReference
  typedef int& (*FP)();
  int& use8(FP fp) { return fp(); }                        // returnTempReference
  int* use10(int& (*fp)()) { return &fp(); }               // returnDanglingLifetime

[autoantwort]

Thanks, all of these are valid. They came from the parsedecl() part of the PR: clearing the reference in the function pointer's ValueType also changed the type of the call fp() from int & to int, which is derived from that ValueType. So the result was treated as a temporary.

I removed that part. The ValueType is unchanged compared to main now, and only Variable::isReference() is fixed. Your examples are added as returnReference30 in testautovariables.cpp, plus typeOf tests for the call.

A self review found two more places that depended on the wrong flags, which are fixed too:

• int& (&f)() (reference to a function returning a reference) was skipped by checkConstVariable only because it was flagged as rvalue reference. It would have got constParameterReference. References to functions are now skipped explicitly.
• Constructors taking Foo& (&f)() changed from move to copy constructor, which lost noCopyConstructor warnings. Function references are no longer taken as copy/move constructor parameters.

(Written by Claude Code on behalf of @autoantwort.)

[dmcppcheck]

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)() and std::string& (*h)(int,int), local function pointers returning references, arrays of them (int& (*arr[2])()), int& (* const p)(), and int& (*&fp)() parameters. Base and PR give the same results, apart from the changes listed below.
• Variable flags from --dump: I checked decltype(g())& a, int& (r), int (&arr)[2], int& (*&pr)(), int&& (*&&pr2)() and alignas(8) int& al. The flags are identical on base and PR. int& (r)(x); is simplified by the tokenizer to int & r ( x ), so the new reset in evaluate() is never reached for it.
• Changed results, all of them improvements:
  • int a2(int (&r)()) { return r(); }: base reports a constParameterReference false positive; the PR does not.
  • Foo(Foo&& (*f)()); is no longer treated as a move constructor, so the PR now reports noExplicitConstructor for it, which is correct.
  • C(C& (&f)()); likewise gets the correct noExplicitConstructor.
• Pre-existing, not caused by the PR: knownPointerToBool on int&& (&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.