whisperity added inline comments.
================ Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:420-465 if (ChecksEnabled[CK_InvalidatedIteratorChecker] && isAccessOperator(Func->getOverloadedOperator())) { // Check for any kind of access of invalidated iterator positions if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) { verifyAccess(C, InstCall->getCXXThisVal()); } else { verifyAccess(C, Call.getArgSVal(0)); ---------------- This is a bit of an organisational comment (and the compiler of course hopefully optimises this out), but could you, for the sake of code readability, break the if-elseif-elseif-elseif chain's common check out into an outer if, and only check for the inner parts? Thinking of something like this: ``` if (ChecksEnabled[CK_InvalidatedIteratorChecker]) { /* yadda */ } if (ChecksEnabled[CK_IteratorRangeChecker]) { X* OOperator = Func->getOverloadedOperator(); if (isIncrementOperator(OOperator)) { /* yadda */ } else if (isDecrementOperator(OOperator)) { /* yadda */ } } /* etc. */ ``` ================ Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1074 - auto &SymMgr = C.getSymbolManager(); - auto &SVB = C.getSValBuilder(); - auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub; - const auto OldOffset = Pos->getOffset(); - const auto intValue = value.getAs<nonloc::ConcreteInt>(); - if (!intValue) + // Incremention or decremention by 0 is never bug + if (isZero(State, Value.castAs<NonLoc>())) ---------------- is never a, also `.` at the end ================ Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1568 +IteratorPosition IteratorChecker::shiftPosition(CheckerContext &C, + OverloadedOperatorKind Op, ---------------- (Minor: I don't like the name of this function, `advancePosition` (akin to `std::advance`) sounds cleaner to my ears.) ================ Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1575 + auto &SVB = C.getSValBuilder(); + auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub; + if (const auto IntDist = Distance.getAs<nonloc::ConcreteInt>()) { ---------------- For the sake of clarity, I think an assert should be introduced her, lest someone ends up shifting the position with `<=`. ================ Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:2329-2330 // Out of range means less than the begin symbol or greater or equal to the // end symbol. ---------------- How does the introduction of `IncludeEnd` change this behaviour? What does the parameter refer to? Repository: rC Clang https://reviews.llvm.org/D53812 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits