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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested changes to this revision.
Szelethus added inline comments.
This revision now requires changes to proceed.


================
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);
----------------
NoQ wrote:
> 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 *`.
+1, I strongly disagree with the overuse of auto. I think project-wide things like the one you mentioned (`SVal::getAs`) are fine, because after googleing it once of twice it genuinely makes the code far more readable. However, where the scope of the type (like in many cases in this file) is very small, line breaks aren't a concern, or the function just simply isn't widely used (and `conjureSymbol` really isn't) we shouldn't use auto.

In general, I'd be just far more conservative: whenever in doubt, just don't use it. Whenever it's anything but super-super obvious, don't use it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61136