baloghadamsoftware marked 9 inline comments as done. baloghadamsoftware added a comment.
In D76361#1929483 <https://reviews.llvm.org/D76361#1929483>, @martong wrote: > I suppose we could get this strange behavior with `std::distance` too? Would > it be worth to handle that as well here? Modeling of `std::distance()` is planned after we model the size of the container properly. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:221 + if (Handler) { + if (!C.wasInlined) { + if (Call.getNumArgs() > 1) { ---------------- martong wrote: > Perhaps putting this hunk into a separate function or lambda could decrease > the nesting level, b/c you could have an early return if there is no > `Handler`. Early return is not possible because the execution must continue if the is no handler. However, I refactored `checkPostCall` now, the handling of both overloaded operators and advance-like functions are moved to separate functions. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:222 + if (!C.wasInlined) { + if (Call.getNumArgs() > 1) { + (this->**Handler)(C, OrigExpr, Call.getReturnValue(), ---------------- martong wrote: > Maybe a comment could be helpful here about the common signature of the > prev/next functions. Like: > ``` > advanceFun ( It it, Distance n = 1 ) > ``` > By the way, can we call `std::advance` with one argument? I added them to the map. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:238 + if (IdInfo) { + if (IdInfo->getName() == "advance") { + if (noChangeInPosition(C, Call.getArgSVal(0), OrigExpr)) { ---------------- martong wrote: > Should this be `__advance`? No. `__advance()` is non-standard, I cannot rely on it. If the function was `std::advance()`, it was inlined, but still did not change the iterator's position means that it did not do its job. Probably the function it calls inside (which could be `__advance()` in most implementations) was not inlined. In this case we model the behavior of `std::advance()` explicitely. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:559 +bool IteratorModeling::noChangeInPosition(CheckerContext &C, SVal Iter, + const Expr *CE) const { ---------------- martong wrote: > Some comments would be helpful here. Also, should we use this function only > with `advance()` or it could be useful perhaps in other context? I renamed the function and added a short comment. Is it OK? ================ Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:571 + const ExplodedNode *N = C.getPredecessor(); + while (N) { + ProgramPoint PP = N->getLocation(); ---------------- martong wrote: > I have a rough presumption that this hunk is a general pattern to get the > previous node for the previous iterator position. So perhaps it would be > useful as a standalone free/static member function too? I moved this to a standalone function and named it accordingly. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76361/new/ https://reviews.llvm.org/D76361 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
