Skip to content

Fix #15203 FN constParameterPointer (push_back() of dereferenced pointer) - #9016

Open
chrchr-github wants to merge 2 commits into
cppcheck-opensource:mainfrom
chrchr-github:chr_15203
Open

chrchr-github wants to merge 2 commits into
cppcheck-opensource:mainfrom
chrchr-github:chr_15203

Conversation

@chrchr-github

Copy link
Copy Markdown
Collaborator

No description provided.

@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 new problems. The change to pindirect only takes away the dereferences that isVariableChanged() has already walked through. It removes the reported false negative and also gives new correct constVariablePointer warnings for local pointers. Base and PR give the same results on the test/cfg files and on part of cppcheck's own sources, and I could not find a false positive. A few closely related forms are still not detected, in base and in the PR: v.push_back(p[0]), v.push_back(*(p + 1)), v.emplace_back(*p) and v.push_back(s->x). When derefs > indirect (for example indirect == 0 and f(*p)), pindirect can now become negative. Library::getArgDirection() treats a negative value as DIR_UNKNOWN, which is the conservative answer. I saw no wrong result from it.

Testing

Builds and tests

  • I built cppcheck and testrunner in pr/, and cppcheck in base/.
  • ./testrunner TestOther TestUninitVar TestNullPointer TestAstUtils TestValueFlow TestVarID TestCondition: 878 tests, 0 failed.
  • The full ./testrunner -q: 5414 tests, 0 failed.

Comparing base and PR on existing code

  • test/cfg/*.c|cpp, each with its own library, --enable=all --inconclusive --check-library: the output is identical.
  • lib/token.cpp, lib/checkother.cpp, lib/library.cpp, lib/symboldatabase.cpp, cli/ and samples/, with --enable=style --inconclusive: the output is identical.

My own test files in scratch/ (a.cpp, b.cpp, c.cpp), run with cppcheck -q --enable=style --inconclusive:

  • New, correct warnings in the PR:
    • void f1(std::vector<int>& v, int* p) { v.push_back(*p); } gives constParameterPointer.
    • int* p = &x; v.push_back(*p); and int* r = p; v.push_back(*r); give constVariablePointer.
    • v.push_back((*s).x) gives constParameterPointer.
  • No new false positives in cases where the pointee really is modified, for example:
    • void g(int*&); void f(int** p){ g(*p); }
    • h(*p) and h(**p) with h(int&)
    • g3(*s) with g3(S&)
    • out.swap(*p)
    • std::strcpy(*s, "a")
    • (*s).m()
    • std::swap(*p, *p)
  • The uninitvar warning on v.push_back(*p) with p = &x and x uninitialised is still reported.
  • Still not detected, in both base and PR: v.push_back(p[0]), v.push_back(*(p + 1)), v.emplace_back(*p) and v.push_back(s->x).

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

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