[PATCH] D29419: [Analyzer] Checker for mismatched iterators

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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).



================
Comment at: lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp:311
+
+void MismatchedIteratorChecker::checkPostStmt(const DeclStmt *DS,
+                                              CheckerContext &C) const {
----------------
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?


================
Comment at: lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp:356
+  for (auto I = SymbolMap.begin(), E = SymbolMap.end(); I != E; ++I) {
+    if (SR.isDead(I->first)) {
+      State = State->remove<IteratorSymbolMap>(I->first);
----------------
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).


https://reviews.llvm.org/D29419