Update coding guidelines regarding use of `auto`

[kristina]

LGTM, I think explicitly giving examples of when it's a good idea to use auto is helpful.

Minor nitpick: May I suggest using IT as an example instead of it because of naming conventions?

[lebedev.ri]

There was also this cover letter:
https://lists.llvm.org/pipermail/llvm-dev/2018-November/127953.html

but generally leaves the rest up to the reviewer, not the contributor.

Exactly, like it should. If the reviewer can't read the code,
then the review will essentially not happen, and the bugs will be missed.

docs/CodingStandards.rst

786–791

Note the subject of the paragraph.
It's not about the code being readable to the author.
It's about code readability for the review/reviewers.

Unbanning more use-cases will not make auto in those cases less opaque.

[aaron.ballman]

docs/CodingStandards.rst

798–800

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.

[Szelethus]

docs/CodingStandards.rst

798–800

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.

[aaron.ballman]

docs/CodingStandards.rst

798–800

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.

[Szelethus]

docs/CodingStandards.rst

798–800

Okay, I may have misunderstood you, thanks for clarifying! :)

[mehdi_amini]

docs/CodingStandards.rst

798–800

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 [...]

FYI that (and all of what @aaron.ballman wrote) represents exactly my state of mind as well.

For instance the fact that it is a for-range loop does not seem like a good discriminator to me: it may or may not be obvious. The guideline where the type is already obvious from the context is expressing this IMO.
For example this seem OK:

for (const auto *Inst : getInstructionsList())

This is less clear-cut:

for (const auto *toDelete : listOfDeletions())

[zturner]

I'm against changes to the auto guidelines. It is more important for code to be readable to other people, not yourself, because there is only 1 of you and there are many other people. What I explicitly do not want to happen is for a reviewer to make a comment like "I find this unreadable" and then to have an author say "well it conforms to the style guidelines, so I'm going to keep it as is".

If people find code unreadable, even for subjective reasons, it has to change. Full stop.

[zturner]

In D54877#1308855, @zturner wrote:

I'm against changes to the auto guidelines. It is more important for code to be readable to other people, not yourself, because there is only 1 of you and there are many other people. What I explicitly do not want to happen is for a reviewer to make a comment like "I find this unreadable" and then to have an author say "well it conforms to the style guidelines, so I'm going to keep it as is".

If people find code unreadable, even for subjective reasons, it has to change. Full stop.

That wording might have been stronger than my actual intent. I'm not against loosening the wording surrounding auto, but at the end of the day, comments that arise in code review should take precedence over what the style guide does or doesn't say when it concerns readability, since it's an actual account of someone stating that they do or don't find something readable, and that's what we're trying to optimize for.

[mehdi_amini]

In D54877#1309017, @zturner wrote:

In D54877#1308855, @zturner wrote:

at the end of the day, comments that arise in code review should take precedence over what the style guide does or doesn't say when it concerns readability, since it's an actual account of someone stating that they do or don't find something readable, and that's what we're trying to optimize for.

FWIW as another data point: I have a totally opposed view on the subject: I rather have clear guidelines *when possible*. Otherwise it is the realm of inconsistency and instability: one reviewers makes you change towards a prefered idiom while the next reviewer will prefer another one.
Just because like you wrote before (with a twist): " because there is only 1 of this reviewers and there are many other people".

[jhenderson]

docs/CodingStandards.rst

786–791

Surely it's about making things more readable for both parties (not specifically the author or the reviewer)? Since developers when maintaining the code have to be able to easily read the code they are about to write/edit, it needs to be readable for them too.

797

As others have said, I agree that Range-based for loops are not a good example in many situations. It may be appropriate in some, but not others.

[asb]

docs/CodingStandards.rst

795–796

This is a change in justification vs the previous wording, and I think the previous justification is more compelling. "the type would have been abstracted away anayways, often behind a container's typedef such as `std::vector<T>::iterator`"

[ruiu]

While reviewing patches for lld, I often request contributors to replace auto with actual types, so I'm probably conservative on using auto than the others. My justification of doing it is that in many situations auto makes easy to write code because the author of the code already know all the types they are using, but it tend to make code hard to read for those who don't know the code well. So, maybe it is a natural consequence that patch authors tend to use auto too frequently and reviewers point that out.

I think overall the description in the change captures my sentiment where we should use auto and where we shouldn't. So overall looking good.

docs/CodingStandards.rst

797

I agree with James. I don't think range-based for loops are not a good example.

[chandlerc]

Please focus the discussion on the llvm-dev thread rather than here.

There does not seem to be clear consensus around the change that should be made there, and we should get that before moving to patch discussion IMO.

[steveire]

Please direct any further discussion or ideas on how the guide could be improved to the mailing list.

docs/CodingStandards.rst

.. code-block:: c++
   bar_map.insert({my_key, my_value});
 
 If you use a braced initializer list when initializing a variable, use an equals before the open curly brace:
 
 .. code-block:: c++
 
   int data[] = {0, 1, 2, 3};
 
 Use ``auto`` Type Deduction to Make Code More Readable
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Some are advocating a policy of "almost always ``auto``" in C++11, however LLVM
-uses a more moderate stance. Use ``auto`` if and only if it makes the code more
-readable or easier to maintain. Don't "almost always" use ``auto``, but do use
-``auto`` with initializers like ``cast<Foo>(...)`` or other places where the
-type is already obvious from the context. Another time when ``auto`` works well
-for these purposes is when the type would have been abstracted away anyways,
-often behind a container's typedef such as ``std::vector<T>::iterator``.
+uses a more moderate stance. Don't "almost always" use ``auto``, but here are
+some cases where use of ``auto`` would make sense:
+
+* Where necessary, such as with lambda variables, inside templates etc.
+* With initializers like ``cast<Foo>(...)``.
+* Where the variable names makes it clear what it is for,
+  such as ``auto it`` instead of  ``std::vector<T>::iterator it``.
+* Range-based for loops.
+* 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.
 
 Beware unnecessary copies with ``auto``
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 The convenience of ``auto`` makes it easy to forget that its default behavior
 is a copy.  Particularly in range-based ``for`` loops, careless copies are
 expensive.