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

[Balogh, Ádám via Phabricator via cfe-commits]

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