Repository navigation
Fix #15155 FN accessMoved (string concatenation using +=) - #8996
chrchr-github wants to merge 8 commits into
Conversation
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
The str().size() == 2 check that detects compound assignment leaves out <<= and >>=, because those operators are 3 characters long. So x <<= 1 / x >>= 1 on a moved object still gets no accessMoved warning, while x += 1 now does. The same gap makes other value types inconsistent: for example, int x; x += y; now gets an uninitvar warning, but int x; x <<= y; stays on legacyUninitvar. Writing it as tok->astParent()->str() != "=" (or Token::Match(parent, "%assign%") && parent->str() != "=") would cover every compound operator.
I found no other new problems. The full testrunner passes. The new nullPointerArithmetic on p += a with a default argument p = 0 matches what base already reports for p + a, so that change makes the two consistent rather than adding a new kind of false positive.
Testing
Built cppcheck and testrunner in pr/, and cppcheck in base/.
./testrunner -q(all tests): 5387 tests, 0 failures.
<<= / >>= are missed (scratch/sh.cpp):
#include <utility>
#include <vector>
struct S { std::vector<int> v; S& operator+=(int); S& operator<<=(int); S& operator>>=(int); };
void s1(S x) { S a{ std::move(x) }; x += 1; }
void s2(S x) { S a{ std::move(x) }; x <<= 1; }
void s3(S x) { S a{ std::move(x) }; x >>= 1; }cppcheck -q --enable=warning --library=std sh.cpp
- base: nothing
- pr: only
sh.cpp:4:37: warning: Access of moved variable 'x'. [accessMoved]. Nothing is reported for lines 5 and 6.
The same inconsistency shows with uninitialized values: in pr, int x; x += y; gives uninitvar, but int x; x <<= y; still gives only legacyUninitvar.
Other checks:
- The PR's case (
std::stringwith+=after a move) now givesaccessMoved, as intended. x = "abc"; x += "d";andx.clear(); x += "d";after a move: no false positive.void f(int a, int *p = 0) { p += a; }: the newnullPointerArithmeticmatches what base already reports forint *q = p + a;.- Integer
x = 0; x += h(y); if (x == 0)andx |= h(y): no new warnings.
Automated review of 5812614 by Claude. The pull request was built and tested locally.
| if (isVariableChanged(tok, getIndirect(tok), getSettings())) { | ||
| if (Token::Match(tok->astParent(), "*|[|.|++|--")) | ||
| if (Token::Match(tok->astParent(), "*|[|.|++|--") || | ||
| (tok->astParent() && tok->astParent()->isAssignmentOp() && tok->astParent()->str().size() == 2)) // compound assignment |
There was a problem hiding this comment.
AI review
str().size() == 2 misses <<= and >>= (both 3 characters). For a moved S x with S& operator<<=(int), x <<= 1; still gets no accessMoved, while x += 1; now does. With uninitialized ints, x += y now gives uninitvar but x <<= y stays legacyUninitvar. tok->astParent()->str() != "=" would cover all compound assignments.
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found no new problems. The earlier review said that <<= and >>= were missed. The current code checks str().size() > 1 on %assign%, which covers every compound operator, and the new tests check both <<= and +=. In my tests the only new findings were real ones:
accessMovedonx += ...after a move, including through reference parameters.uninitvarreplacinglegacyUninitvarfor+=,*=and^=on uninitialized scalars and pointers.nullPointerArithmeticRedundantCheckforif (!p) {} p += n;. The base version already reports this for the equivalentp + n.
I found no new false positives in the integer, symbolic or condition cases I tried.
Testing
I built cppcheck and testrunner in pr/ and cppcheck in base/.
./testrunner -q(all tests): 0 failures.
I compared base and PR with cppcheck -q --enable=all --inconclusive --library=std on my own files in scratch/. The results:
Changed (all correct):
void f1(std::string x){ std::string a{std::move(x)}; x += "abc"; }: the PR now reportsaccessMoved.void f12(std::string &r){ std::string s(std::move(r)); r += "x"; }: the PR now reportsaccessMoved.int x; if (b) x = 1; x += 2;,float f; f *= 2.0f;,unsigned u; u ^= a;andint *q; q += 1;: these change fromlegacyUninitvartouninitvar.void f2(char *p, int n){ if (!p) {} p += n; }: the PR now reportsnullPointerArithmeticRedundantCheck. Base already reports the same forchar *q = p + n;, so the two forms now agree.
Unchanged:
- Known and symbolic integer values through compound assignment:
x = 5; x += 1; 10/(x-6)(zerodiv),int y = x; x += 1; if (y == x),x >>= 1,x *= zande += non pointers. - Container size after
s += "a". v = {}; v[0] += 1(containerOutOfBounds).q += 0on a null pointer: no warning.p += nafterif (!p) return;: no warning.- A tokenizer loop that does
push_back(std::move(cur)); cur = "";and thencur += c: no false positive.
Message change: for struct Acc { int v; Acc& operator+=(int i){ v = i; return *this; } }; Acc a; a += 1;, base reports Uninitialized variable: a.v and the PR reports Uninitialized variable: a. Both versions warn here, and the operator never reads v, so neither warning is right. Only the reported name changes.
Automated review of 811a54c by Claude. The pull request was built and tested locally.
|
Test results for commit 811a54c (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8996/ 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 <<=/>>= gap from the earlier reviews is fixed: vf_analyzers.cpp now checks %assign% with str().size() > 1, which matches every compound operator.
The new isVariableChanged change in astutils.cpp treats a compound assignment as a change of the pointer itself but not of what it points to. I tested it on pointer arithmetic, int** and int*&, iterators, containers and buffers that are filled through an advanced pointer. It added no false positives or false negatives. It removed two false positives that base reports (below), and it found one real uninitialized read.
Earlier comments
| # | Comment | By | Where | Status |
|---|---|---|---|---|
| 1 | Compound-assignment check str().size() == 2 misses <<= and >>= |
AI review | general | ✅ Fixed |
| 2 | str().size() == 2 misses <<=/>>= (accessMoved, uninitvar) |
AI review | lib/vf_analyzers.cpp:232 |
✅ Fixed |
Testing
I built cppcheck and testrunner in pr/ and cppcheck in base/.
./testrunner -q(all tests): 5418 tests, 0 failures.TestUninitVar,TestValueFlow,TestNullPointerandTestOtheralso pass when run on their own.
I compared base and PR with cppcheck -q --enable=all --inconclusive --library=std on my files in scratch/.
Results that changed (all improvements):
int h1() { char buf[10]; char* p = buf; p += 2; return *p; }: the PR now reportsuninitvaron*p. That is correct.int k3() { char buf[10]; char* p = buf; p += 2; *p = 1; return buf[2]; }: base wrongly reportsuninitvar: buf. The PR reports nothing.int h5() { int x = 0; int* p = &x; p += 0; *p = 1; return 10 / x; }: base wrongly reportszerodiv. The PR reports nothing.
Results that did not change:
- Pointers: I checked
constParameterPointer/constVariablePointeron*p += 1,**p += 1,p[0] += 1,(*p) += 1,t->m += 1,*s += 1with an overloadedoperator+=,int*& r = *p; r += 1,*pp += 1,pp[0] += 1,int*& r; r += 1,t->p += 1anda[1] += 2onint* a[]. Base and PR give the same results for all of them. - Containers:
invalidContainerafters += "abc"is still reported forc_str(),data(),begin()and&s[0]. - Buffers filled through an advanced pointer: loops that write
*p = 0; p += 1,memset(p, …)andfill(p)give the same results as base. - accessMoved: it is reported for
x += …after a move. There is no false positive afterx = "a";orx.clear();, or when the move is on another path.
Automated review of a2b79cc by Claude. The pull request was built and tested locally.
No description provided.