A new checker to check whether multiple iterators which should refer to the same container refer to different ones. Here we include comparisons, constructors which take an iterator pair and any template function which takes iterator pair(s).
Diff Detail
Event Timeline
Hello, thanks for another useful checker! I make quite a few of these mistakes regularly.
This one looks similar to the IteratorPastEnd checker, so much that i'd definitely advice re-using some code. At the very least, functions like isIterator() should definitely deserve a header somewhere in include/clang/StaticAnalyzer/Checkers.
Also, did you consider merging these checkers together into one file? Just because they have so much in common.
The usual stuff: do you already have any quality statistics, eg. how many warnings did you see emitted by this checker, how many of them are true/false positives. With that, better warning messages, and a bug reporter visitor to highlight the origin of the misplaced iterator, i think this checker should be enabled by default (go out of alpha).
lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp | ||
---|---|---|
311 | Hmm. Now i suspect that the checkBind() callback should have covered this, both here and in the previous checker. Did you try using that instead, and see if other callbacks are covered by checkBind() as well? | |
356 | I'd consider !isLive() here due to, uhm, subtle differences we currently have between these two (there's a hard-to-fix bug that causes some symbols to never make it to the dead set, see D18860). |
Yes. Actually these two checkers served for me as prototypes, but it turned out quite early that if I want to implement the most important iterator checker, thus the checker for invalidated iterators it requires a tracker structure that includes all the data we need for these two checkers. So it would be a wast of resources to duplicate these data. So now I am also working on the merged version.
lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp | ||
---|---|---|
311 | You are right. It seems to eliminate the need for checking C++ construct expressions, declarations with initial values and assignments. |
lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp | ||
---|---|---|
376 | This can be rewritten as: if (auto *N = C.generateNonFatalErrorNode(State, &Tag)) reportMismatchedBug("Iterator access mismatched.", Iter1, C, N); |
So it would be a wast of resources to duplicate these data. So now I am also working on the merged version.
Would it make sense to just resume the review on the merged patch?
Hmm. Now i suspect that the checkBind() callback should have covered this, both here and in the previous checker. Did you try using that instead, and see if other callbacks are covered by checkBind() as well?