[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:
> Szelethus wrote:
> > 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.
> > 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.
> 
> I was speaking in generalities before; my feelings on the static analyzer are a bit more complicated and are perhaps orthogonal in some ways. We let different projects have different coding styles, but those projects are usually a separate repository (perhaps with a separate mailing list, etc). The CSA isn't a separate code base in any meaningful sense, which is why I get a bit itchy saying, "this is the rule, except in this one corner of the product." However, this (and other situations in the CSA) is a long-standing, purposeful deviation from how the rest of the project works and changing that would be kind of a separate discussion from this one, IMO. That said, this is still good discussion material for how we want to treat `auto` in the rest of the code base because it's a realistic developer scenario.
> 
> > 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.
> 
> Thoughts on per-subsystem exceptions aside, I agree that breathing room is needed. These are style guidelines, not style mandates. :-) This is why I actually prefer some of the original words: `or other places where the type is already obvious from the context.` For the CSA, "obvious from the context" means `optional<SVal>` can be spelled `auto`.
Okay, I may have misunderstood you, thanks for clarifying! :)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54877