Skip to content

Fix #15185 FP constParameterPointer with QMap::insert - #9011

Open
chrchr-github wants to merge 6 commits into
cppcheck-opensource:mainfrom
chrchr-github:chr_15185
Open

chrchr-github wants to merge 6 commits into
cppcheck-opensource:mainfrom
chrchr-github:chr_15185

Conversation

@chrchr-github

Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread cfg/std.cfg
<use-retval/>
<const/>
<arg nr="1" direction="in">
<arg nr="1" direction="in" indirect="0">

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There already is a test:

return m.count(p) > 0;
see https://trac.cppcheck.net/ticket/15194

@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

The fix works for the functions it changes. The same constParameterPointer false positive is still there for QMap functions that this PR either touches or sits right next to:

  • QMap::value argument 2 (the default value) is unchanged.
  • QMap::key argument 2 (the default key) is unchanged.
  • QMap::remove is unchanged, and so is QHash::remove.

Code like m.value(k, d), m.key(v, d) or m.remove(k) with int* keys or values still warns. I found no crashes or new false positives. One side effect of indirect="0": passing a dereferenced pointer to these functions, e.g. m.count(*k) with std::map<int,int>, no longer gives constParameterPointer or constVariablePointer. Existing entries that use indirect="0", such as std::vector::push_back, already behave this way.

Testing

  • Built cppcheck for pr/ and base/. I did not build testrunner because the PR only changes cfg files and cfg tests.
  • test/cfg/qt.cpp and test/cfg/std.cpp pass with the PR cppcheck, using the runtests.sh options (--check-library --check-level=exhaustive --enable=style,information --inconclusive --inline-suppr). There were no unexpected warnings and the exit code was 0.

False positives that remain (scratch/t.cpp):

#include <QMap>
void f_remove(QMap<int*, int*>& m, int* k) { m.remove(k); }
int* f_value2(const QMap<int*, int*>& m, int* k, int* d) { return m.value(k, d); }
int* f_key2(const QMap<int*, int*>& m, int* v, int* d) { return m.key(v, d); }

Command: cppcheck --library=qt -q --enable=style --inconclusive t.cpp

PR output:

4:constParameterPointer:Parameter 'k' can be declared as pointer to const
5:constParameterPointer:Parameter 'd' can be declared as pointer to const
6:constParameterPointer:Parameter 'd' can be declared as pointer to const

QHash::remove(k) gives the same warning: void b(QHash<int*, int*>& m, int* k) { m.remove(k); }.

Side effect (scratch/u.cpp, same command):

int g1(const std::map<int, int>& m, int* k) { return m.count(*k); }
bool g2(const QMap<int, int>& m, int* k) { return m.contains(*k); }
int g3(const std::map<int, int>& m) { int x = 0; int* k = &x; return m.count(*k); }
  • base: constParameterPointer for k on lines 3 and 4, and constVariablePointer for k on line 5.
  • PR: no warnings.

Existing indirect="0" entries already behave this way: std::vector::push_back(*k) gives no warning in base either.

Other checks: uninitvar for uninitialized keys and values passed to count, value and insert is reported the same in base and PR. The 3-argument insert(pos, k, v) and std::set::count no longer give the false positive.


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

Comment thread cfg/qt.cfg Resolved
Comment thread cfg/qt.cfg Resolved

@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 false positives raised in the earlier AI review are fixed in the current version. Those were QMap::value argument 2, QMap::key argument 2, QMap::remove and QHash::remove, and each one now has indirect="0" and a test in test/cfg/qt.cpp. The other QMap and QHash lookup and insert functions I tried with int* keys and values give no constParameterPointer either. Uninitialized keys are still reported. The earlier review already noted that indirect="0" no longer warns for a dereferenced pointer argument such as m.count(*k).

Earlier comments

# Comment By Where Status
1 QMap::value arg 2 still gives constParameterPointer FP AI review general ✅ Fixed
2 QMap::key arg 2 still gives constParameterPointer FP AI review general ✅ Fixed
3 QMap::remove / QHash::remove still give constParameterPointer FP AI review general ✅ Fixed
4 QMap::key arg 2 lacks indirect="0" AI review cfg/qt.cfg:1065 ✅ Fixed
5 QMap::value arg 2, QMap::remove and QHash::remove lack indirect="0" AI review cfg/qt.cfg:1100 ✅ Fixed

Testing

  • I built cppcheck in pr/. I did not build testrunner because the PR only changes cfg files and cfg tests. I did not build base/ either, since no comparison was needed.

  • I ran test/cfg/qt.cpp (with --library=qt) and test/cfg/std.cpp (with --library=std). Both used --check-library --check-level=exhaustive --enable=style,information --inconclusive --inline-suppr --error-exitcode=1, and both exited with 0. The only messages were unmatchedSuppression information messages, which runtests.sh normally handles.

  • I wrote scratch/h.cpp to cover functions that the PR does not touch, plus one uninitvar case:

    • QHash::contains, find, insert, and value(k, d)
    • QMap::take, values(k) and constFind, all with int* keys and values
    • an uninitialized key passed to QMap::insert and QMap::remove

    I ran cppcheck --library=qt -q --enable=style --inconclusive h.cpp. There was no constParameterPointer warning, and uninitvar was still reported for the uninitialized key.


Automated review of a17f7dd 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