[PATCH] D54877: Update coding guidelines regarding use of `auto`

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

Szelethus added inline comments.


================
Comment at: docs/CodingStandards.rst:798-800
+* Where the instance is only needed to perform a validity check,
+  such as a non-empty ``optional``, calling ``isValid()``, ``isNone()``,
+  comparing in an algorithm with an ``end()`` iterator etc.
----------------
aaron.ballman wrote:
> This has me concerned as a reviewer. One benefit to the current `auto` guidance is that a reviewer outside of an IDE can quickly determine what types are involved without having to play "hunt the type" to work back through code to find it. You mention `isValid()`, which is a great example of why I'm opposed -- some types have paired `isValid()` and `isInvalid()` member functions (like `SourceLocation`) while others only have `isValid()` and no `isInvalid()` (like `SMLoc`). When reviewing code, it is useful for me to tell the type at a glance so I can suggest using `Loc.isInvalid()` rather than `!Loc.isValid()`. Both of these types are related to source locations, but knowing the concrete type at a glance is useful.
> 
> As another example -- the mention of `optional` has come up during code reviews where more than one code reviewer was unaware that a type was optional because the optionality was hidden behind an `auto` -- it came up because a reviewer was asking for it to instead be spelled `const auto *` because the usage looked pointer-like.
> 
> I'm all for making the documentation more clear about when it's appropriate to use `auto`. For instance, we are consistently inconsistent with whether we write `for (const auto *F : foos())` or `const Foo *F : foos())` and I think clarifying that would be beneficial. However, I'm not convinced that we want to allow using `auto` in more places than we already do. Not all text editors are great at giving you type information at a glance and web browsers (where we do the majority of our code reviewing) certainly don't give you type information at a glance. I think code maintainability is paramount and whether you can understand code you are reading over is an important indicator of maintainability.
> I'm all for making the documentation more clear about when it's appropriate to use auto. [...] However, I'm not convinced that we want to allow using auto in more places than we already do.

I see your point here, but I disagree that we shouldn't allow using auto for optional types, if the context is obvious enough. For example, if I'm writing a new `static` function inside a TU, that may fail and return with `llvm::None`, I would strongly prefer typing out the type. However, when that function or method is the backbone of a whole library, like in the Static Analyzer, which if I'm not mistaken is where your main concern originates from, using `auto` for this is I think justified.
```
auto LocVal = SomeVal.getAs<loc::MemRegionVal>();
if (LocVal)
  // ...
```
`SVal` is among (if not the) most fundamental types we're working with, and I think writing out the whole type (which often results with a line break, if the variable names are verbose enough or we're in 4-6 spaces indented already) would greatly reduce readability, without providing enough value in return:

```
Optional<loc::MemRegionVal> LocVal =
    SomeVal.getAs<loc::MemRegionVal>();
if (LocVal)
  // ...
```
Unless we want to make exceptions for certain projects, which if I recall correctly didn't sound too great to you, I think leaving some breathing room in the rule is fine.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54877