[PATCH] D61136: [Analyzer] IteratorChecker - Ensure end()>=begin() and refactor begin and end symbol creation

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D61136#1480023 <https://reviews.llvm.org/D61136#1480023>, @baloghadamsoftware wrote:

> Abstract iterator positions are represented by symbolic expressions because we must be able to do simple calculations with them. This has nothing to do with iterator objects represented as symbols. Or what do you mean exactly by "replacing position symbols with `SymbolMetadata`"?


I mean, use `SymbolMetadata` wherever you use `SymbolConjured`. Because metadata is attached to objects (i.e., it takes a region as part of its identity), it's problematic to use with iterators represented as symbols.

Though i'd also consider the opposite direction which seems to be even more promising: re-do `SymbolMetadata` so that it took a symbol instead of a region as part of its identity, and then attach "identity symbols" to all C++ objects, so that we didn't ever have to track iterators by their regions, but instead we could make `IteratorChecker` operate more like `SimpleStreamChecker`. This still requires re-doing all the conjuring, so keeping all the conjuring in one place is still a good idea.



================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1929-1930
+
+  auto &SymMgr = State->getSymbolManager();
+  auto Sym = SymMgr.conjureSymbol(E, LCtx, T, BlockCount, "end");
+  State = assumeNoOverflow(State, Sym, 4);
----------------
baloghadamsoftware wrote:
> NoQ wrote:
> > This is a bit more `auto` than allowed by [[ https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | our coding standards ]] (it pretty much disallows `auto` unless it's some sort of `dyn_cast` (`getAs`) or an iterator.
> I can add the type, of course. However, until now I understood and it is also in the coding standard that "other places where the type is already obvious from the context". For me it is obvious that `conjureSymbol()` returns a `SymbolRef`. Even more obvious is the `getSymbolManager()` returns a `SymbolManager`.
[[ https://reviews.llvm.org/D33672?id=171506#inline-475812 | Here is a discussion on this matter that i had recently. ]] It was hard enough to convince people that the code below follows the coding standards:
```lang=c++
auto DV = V.getAs<DefinedOrUnknownSVal>();
```

Also, `conjuredSymbol()` in fact returns a `const SymbolConjured *`, which is a more specialized type. This probably deserves at least a `const auto *`.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:2247
 
+ProgramStateRef ensureNonNegativeDiff(ProgramStateRef State, SymbolRef Sym1,
+                                      SymbolRef Sym2) {
----------------
baloghadamsoftware wrote:
> NoQ wrote:
> > This looks like a new feature. Is it testable?
> I am not sure I can test it alone. Maybe I should leave it for now and add it in another patch together with modelling of `empty()` or `size()`. Then I should also rename this patch which remains pure refactoring.
Yup, i think it's good to keep NFC commits and features apart.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61136