Skip to content

Partial fix for #15174 (ValueFlow speedups: lifetime of non-local variables) - #9014

Open
danmar wants to merge 1 commit into
cppcheck-opensource:mainfrom
cppchecksolutions:fix-15174-commit1
Open

danmar wants to merge 1 commit into
cppcheck-opensource:mainfrom
cppchecksolutions:fix-15174-commit1

Conversation

@danmar

@danmar danmar commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator

valueFlowForwardLifetime: do not forward lifetime of non-local variable past end of function

For a global variable the scope body end is null so the lifetime was forwarded through the rest of the file, into unrelated functions.

colobot restext.cpp: ValueFlow 615s => 5s.
In 10000 Debian packages (--check-level=exhaustive) this removes 11 false positives (danglingLifetime, objectIndex) and adds no warnings.

…iables)

valueFlowForwardLifetime: do not forward lifetime of non-local variable past end of function

For a global variable the scope body end is null so the lifetime was
forwarded through the rest of the file, into unrelated functions.

colobot restext.cpp: ValueFlow 615s => 5s.
In 10000 Debian packages (--check-level=exhaustive) this removes 11
false positives (danglingLifetime, objectIndex) and adds no warnings.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@dmcppcheck dmcppcheck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

I found no problems. Limiting forward analysis to the end of the enclosing function when the variable is non-local fits how ValueFlow::getEndOfExprScope is already used in the assignment branch of valueFlowForwardLifetime. When the token is at global scope (for example a global initializer), the end stays null, so the old behaviour is kept. Inside a lambda the forwarding now stops at the end of the lambda, which is correct because no code after it runs in sequence with the lambda body. My tests turned up no new false negatives or other differences in results.

Testing

  • I built cppcheck and testrunner in pr/, and cppcheck in base/.

  • I ran ./testrunner for TestValueFlow, TestAutoVariables, TestStl, TestBufferOverrun, TestNullPointer, TestUninitVar and TestOther. 0 failures in each.

  • I wrote scratch/a.cpp and checked it with base and PR using cppcheck -q --enable=all --inconclusive --check-level=exhaustive. It has true positives that involve non-local variables:

    • invalidContainer on global vectors and strings (&gv[0], gv.begin(), gs.c_str(), followed by push_back, clear or assignment), including inside a lambda.
    • invalidContainer on class members, in an inline member function and in an out-of-class one.
    • invalidContainer on a vector in a namespace.
    • A global pointer to an out-of-scope local (danglingLifetime / invalidLifetime).
    • pointerOutOfBounds on a global array.

    Base and PR gave identical output.

  • I ran base and PR on test/cfg/std.cpp, qt.cpp, wxwidgets.cpp, windows.cpp and samples/*/bad.c* with the same options. The output was identical: 1692 lines each.

  • I could not reproduce the colobot timing improvement because it needs the colobot sources, and the container has no network access.


Automated review of afb5cba by Claude. The pull request was built and tested locally.

@dmcppcheck

Copy link
Copy Markdown
Contributor

Test results for commit afb5cba (tools/test-my-pr.py, main compared to this PR):

Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-9014/
packages: 1000
Differing warnings: 1 (+0 -1)
AI review: 1 improvement (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-9014/2026-11-11_03-42-47-afb5cbae18f5-review.txt)
Timing: -0.1% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-9014/2026-11-11_03-42-47-afb5cbae18f5_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

danmar commented Nov 15, 2026

Copy link
Copy Markdown
Collaborator Author

@pfultz2 any opinions? This does not seem to have a big impact in general on speed. But it fixes some false positives.

The reduced code for the test:

#include <list>
struct Buffer {
    Buffer(const char* buf) {
        m_buf = 0;
    }
    char* m_buf;
};
struct Stream {
    std::list<Buffer> m_stream;
    void push_front(char* data) {
        m_stream.push_front(Buffer(data));  // <- True positive , reported by both
    }
    void write(char* data) {
        m_stream.push_back(Buffer(data));     // <- False positive reported with main
    }
};
struct Socket {
    Stream m_write;
    void process() {
        char ptr[10];
        m_write.push_front(&ptr[1]);
    }
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants