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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
