[PATCH] D76361: [Analyzer] Iterator Modeling - Model `std::advance()`, `std::prev()` and `std::next()`

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

LGTM!



================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:221
+    if (Handler) {
+      if (!C.wasInlined) {
+        if (Call.getNumArgs() > 1) {
----------------
baloghadamsoftware wrote:
> 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.
Thanks, that's much better structured this way.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:559
 
+bool IteratorModeling::noChangeInPosition(CheckerContext &C, SVal Iter,
+                                          const Expr *CE) const {
----------------
baloghadamsoftware wrote:
> 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?
Yeah, looks ok.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:571
+  const ExplodedNode *N = C.getPredecessor();
+  while (N) {
+    ProgramPoint PP = N->getLocation();
----------------
baloghadamsoftware wrote:
> 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.
Thanks, it's easier to read the code this way.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76361/new/

https://reviews.llvm.org/D76361