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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
