[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DebugIteratorModeling.cpp:107-108
+          BR.markInteresting(Iter);
+          if (const auto &LCV = Iter.getAs<nonloc::LazyCompoundVal>()) {
+            BR.markInteresting(LCV->getRegion());
+          }
----------------
What region would it be in the common scenario? Will it be the argument region or the region from which the value was copied into the argument region? Can we say explicitly "let's just take the argument region" because it's clearly the right thing to do?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:541-542
+        BR.markInteresting(It1);
+        if (const auto &LCV1 = It1.getAs<nonloc::LazyCompoundVal>()) {
+          BR.markInteresting(LCV1->getRegion());
+        }
----------------
I'm opposed to this code for the same reason that i'm opposed to it in the debug checker. Parent region is an undocumented implementation detail of `RegionStore`. It is supposed to be immaterial to the user. You should not rely on its exact value.

@baloghadamsoftware Can we eliminate all such code from iterator checkers and instead identify all iterators by regions in which they're stored? Does my improved C++ support help with this anyhow whenever it kicks in?


================
Comment at: clang/test/Analysis/iterator-modelling.cpp:69
 
-  auto j = ++i;
+  auto j = ++i; // expected-note 2{{Iterator 'i' incremented by 1}}
 
----------------
Let's also add a test for a simple copy, i.e. `auto j = i;`. Does it say `Iterator 'i' copied`? What about `moved`?


================
Comment at: clang/test/Analysis/iterator-range.cpp:33
 void deref_end(const std::vector<int> &V) {
   auto i = V.end();
   *i; // expected-warning{{Past-the-end iterator dereferenced}}
----------------
This line ultimately needs a note as well, i.e. `Iterator points to end of container 'V'` or something like that.


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

https://reviews.llvm.org/D75677