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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman 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.
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54877