llvm dev - Dec 2018 - RFC: Modernizing our use of auto

I'm not advocating AAA.

However this is a proposal for more modern thinking regarding the 
permissiveness of auto in LLVM codebases.

Currently the rule on the use of auto is here:

 
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

It is quite strict. It allows the use of auto for

* lambdas
* iterators because they are long to type
* casts to the type

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

The notes about "more readable", "easier to maintain",
"obvious from the
context" are very subjective.

I think these guidelines need to be made more clear both in that 
subjective sense, but also in the sense of use of new/recent language 
and library features, given that LLVM will allow C++17 features in a few 
months. For example:

* Can we use auto in the initializer of range-for statements?
* Can we use auto in the initializer of if() statements?
* Can we use auto with llvm:Optional instances?
* Can we use auto in c++14 lambda arguments with llvm::find_if(C, 
[](const auto& i) { ... }) for example?
* We need to use auto for structured bindings, if those are used.

I know some people are very opposed to use of auto except in exceptional 
circumstances, but

* Some features force it a bit
* Elsewhere it is becoming more common for it to be reasonable to use. 
People are not  going as far as AAA, but auto is generally used in 
blogs, slides, and other peoples code (eg 
https://skebanga.github.io/if-with-initializer/). It's becoming more 
normalized without becoming extreme.
* Our potential contributors will not see a problem with using it in the 
above cases

So, we might benefit from somewhat more-modern guidelines.

Reviewers are very subjective and that means it is hard to predict what 
the requirements will be, depending on the reviewer. This is confusing 
for contributors.

For example, I was told by a reviewer that auto should *not* be used in 
the initalizer of a range for loop.  However, even the next section of 
the coding guidelines suggest that is ok:

 
https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto

So, perhaps the guidelines for reviewers needs to adapt, or the coding 
guidelines need to adapt.

Currently, reviewers reject code of the form

   void foo()
   {
     auto ParserInstance = Parser::getFromArgs(Args);
     if (ParserInstance)
       obj.someApi(ParserInstance);
   }

as unreadable while

   void foo()
   {
     obj.otherApi(Parser::getFromArgs(Args));
   }

is fine. It would be good to have an answer in the coding guidelines to 
whether

   void foo()
   {
     if (auto ParserInstance = Parser::getFromArgs(Args); ParserInstance)
       obj.someApi(ParserInstance);
   }

is ok or not.

In the case that came up in review for me, the code I submitted is

template <typename BaseT, typename DerivedT>
void registerIfNodeMatcher(...) {
   auto NodeKind = ASTNodeKind::getFromNodeKind<DerivedT>();
   if (!NodeKind.isNone())
     NodeCtors[NodeKind] = std::make_pair(MatcherName, Descriptor);
}

but it was rejected as unreadable and I was given a link to the coding 
guidelines.

'NodeKind' appears three times in one line in that code and the 
requirement is to add it again. ASTNodeKind is a very commonly used 
class in that part of Clang, so it should be familiar enough. We only 
need the local variable at all so that we can do a validity check. There 
is no reasonable need to put the type there or even to care what exactly 
the type is. All we need to know is that we check the validity of the 
instance before using it.

I'd like to update the coding guidelines so that code such as

void foo(...) {
   auto Bar = SomeAPI();
   if (/* validity condition */)
     OtherAPI(Bar);
}

is unambiguously acceptable in the coding guidelines.

That kind of thing is coming up as Optional is used more in the 
codebase, but also with objects which have a validity check such as 
isNone(), isValid(), isNull() or just `!= nullptr` etc.

I'd also like to update them so that

llvm::Optional<std::pair<std::string, MatcherCtor>>
getNodeConstructorType(ASTNodeKind targetType) {
   auto const &ctors = RegistryData->nodeConstructors();
   auto it = llvm::find_if(
       ctors, [targetType](const NodeConstructorMap::value_type &ctor) {
         return ctor.first.isSame(targetType);
       });
   if (it == ctors.end())
     return llvm::None;
   return it->second;
}

is acceptable. The `auto it` is already acceptable, but the `auto const& 
ctors` depends on an interpretation of the guideline and was rejected in 
the review I submitted.

In that code, we only create the local `ctors` variable because we need 
to compare the `end` iterator after the algorithm.  If auto is ok to use 
in range-for loops, then reasonably it should be ok in the above code too.

Naturally, the guidelines should only be changed if there is consensus 
to change them. No one is trying to force anything, but I'm trying to 
address an inconsistency in reviews and a pain point for a new 
contributor. I'm trying to see if I can make the experience easier by 
updating the guidelines. Reviewers should be able to fall back to a good 
guideline, even if it differs from their subjective taste.

This is a pain point for me as a relatively new contributor to LLVM 
because sometimes reviewers have differing opinions. We shouldn't be 
putting up barriers to new contributors, but trying to remove them 
instead. I put up

  https://reviews.llvm.org/D54877

to try to start the conversation, so I'm interested in opinions!

Thanks,

Stephen.

On Nov 25, 2018, at 6:43 AM, Stephen Kelly via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> 
> I'm not advocating AAA.
> 
> However this is a proposal for more modern thinking regarding the
permissiveness of auto in LLVM codebases.
> 
> Currently the rule on the use of auto is here:
> 
>
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
> 
> It is quite strict.
Sure, the approach of the standards doc is to be quite strict but leave open the
door for reasonable human discretion during code review.  That said, you’re
right that this should be improved.
> It allows the use of auto for
> 
> * lambdas
> * iterators because they are long to type
> * casts to the type
> 
> but generally leaves the rest up to the reviewer, not the contributor.
> 
> The notes about "more readable", "easier to maintain",
"obvious from the context" are very subjective.
Indeed, intentionally so.  Being accurate is difficult.
> I think these guidelines need to be made more clear both in that subjective
sense, but also in the sense of use of new/recent language and library features,
given that LLVM will allow C++17 features in a few months.
Sure, here is MHO (not intended to be prescriptive):
> For example:
> 
> * Can we use auto in the initializer of range-for statements?
Yes, if it is contextually obvious what the element type of the container is.
> * Can we use auto in the initializer of if() statements?
Yes, if it is contextually obvious what the type is, because it is the result of
a dyncast etc.  No in general.
> * Can we use auto with llvm:Optional instances?
Generally no IMO, because the cases that produce optional are not obvious.
> * Can we use auto in c++14 lambda arguments with llvm::find_if(C, [](const
auto& i) { ... }) for example?
> * We need to use auto for structured bindings, if those are used.
These both get into “yes, if the type is contextually obvious”.  I’m not sure
how to specify this though.

Perhaps the rule came be rewritten more generally, as in “auto should generally
only be used when the inferred type is obvious from context, e.g. as a result of
a cast like dyn_cast or as a result of constructing a value with a constructor
containing a type name.”?
> So, perhaps the guidelines for reviewers needs to adapt, or the coding
guidelines need to adapt.
+1 for adapting to changes in the ecosystem.
> Currently, reviewers reject code of the form
> 
>  void foo()
>  {
>    auto ParserInstance = Parser::getFromArgs(Args);
This seems like it could be considered a named constructor.
> In the case that came up in review for me, the code I submitted is
> 
> template <typename BaseT, typename DerivedT>
> void registerIfNodeMatcher(...) {
>  auto NodeKind = ASTNodeKind::getFromNodeKind<DerivedT>();
>  if (!NodeKind.isNone())
>    NodeCtors[NodeKind] = std::make_pair(MatcherName, Descriptor);
> }
> 
> but it was rejected as unreadable and I was given a link to the coding
guidelines.
I agree, this seems overly strict.
> That kind of thing is coming up as Optional is used more in the codebase,
but also with objects which have a validity check such as isNone(), isValid(),
isNull() or just `!= nullptr` etc.
> 
> I'd also like to update them so that
> 
> llvm::Optional<std::pair<std::string, MatcherCtor>>
> getNodeConstructorType(ASTNodeKind targetType) {
>  auto const &ctors = RegistryData->nodeConstructors();
>  auto it = llvm::find_if(
>      ctors, [targetType](const NodeConstructorMap::value_type &ctor) {
>        return ctor.first.isSame(targetType);
>      });
>  if (it == ctors.end())
>    return llvm::None;
>  return it->second;
> }
> 
> is acceptable. The `auto it` is already acceptable, but the `auto
const& ctors` depends on an interpretation of the guideline and was rejected
in the review I submitted.
This seems like a step too far from me.  I don’t know what nodeConstructors is
here at all, and you’ve erased all type information about what you’re dealing
with.

-Chris

Le mer. 28 nov. 2018 à 09:26, Chris Lattner via llvm-dev
<llvm-dev at lists.llvm.org> a écrit :
>
> On Nov 25, 2018, at 6:43 AM, Stephen Kelly via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> >
> >
> > I'm not advocating AAA.
> >
> > However this is a proposal for more modern thinking regarding the
permissiveness of auto in LLVM codebases.
> >
> > Currently the rule on the use of auto is here:
> >
> >
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
> >
> > It is quite strict.
>
> Sure, the approach of the standards doc is to be quite strict but leave
open the door for reasonable human discretion during code review.  That said,
you’re right that this should be improved.
>
> > It allows the use of auto for
> >
> > * lambdas
> > * iterators because they are long to type
> > * casts to the type
> >
> > but generally leaves the rest up to the reviewer, not the contributor.
> >
> > The notes about "more readable", "easier to
maintain", "obvious from the context" are very subjective.
>
> Indeed, intentionally so.  Being accurate is difficult.
>
> > I think these guidelines need to be made more clear both in that
subjective sense, but also in the sense of use of new/recent language and
library features, given that LLVM will allow C++17 features in a few months.
>
> Sure, here is MHO (not intended to be prescriptive):
>
> > For example:
> >
> > * Can we use auto in the initializer of range-for statements?
>
> Yes, if it is contextually obvious what the element type of the container
is.
>
> > * Can we use auto in the initializer of if() statements?
>
> Yes, if it is contextually obvious what the type is, because it is the
result of a dyncast etc.  No in general.
>
> > * Can we use auto with llvm:Optional instances?
>
> Generally no IMO, because the cases that produce optional are not obvious.
>
> > * Can we use auto in c++14 lambda arguments with llvm::find_if(C,
[](const auto& i) { ... }) for example?
> > * We need to use auto for structured bindings, if those are used.
>
> These both get into “yes, if the type is contextually obvious”.  I’m not
sure how to specify this though.
>
> Perhaps the rule came be rewritten more generally, as in “auto should
generally only be used when the inferred type is obvious from context, e.g. as a
result of a cast like dyn_cast or as a result of constructing a value with a
constructor containing a type name.”?
FWIW
+1
>
> > So, perhaps the guidelines for reviewers needs to adapt, or the coding
guidelines need to adapt.
>
> +1 for adapting to changes in the ecosystem.
>
> > Currently, reviewers reject code of the form
> >
> >  void foo()
> >  {
> >    auto ParserInstance = Parser::getFromArgs(Args);
>
> This seems like it could be considered a named constructor.
>
> > In the case that came up in review for me, the code I submitted is
> >
> > template <typename BaseT, typename DerivedT>
> > void registerIfNodeMatcher(...) {
> >  auto NodeKind = ASTNodeKind::getFromNodeKind<DerivedT>();
> >  if (!NodeKind.isNone())
> >    NodeCtors[NodeKind] = std::make_pair(MatcherName, Descriptor);
> > }
> >
> > but it was rejected as unreadable and I was given a link to the coding
guidelines.
>
> I agree, this seems overly strict.
>
> > That kind of thing is coming up as Optional is used more in the
codebase, but also with objects which have a validity check such as isNone(),
isValid(), isNull() or just `!= nullptr` etc.
> >
> > I'd also like to update them so that
> >
> > llvm::Optional<std::pair<std::string, MatcherCtor>>
> > getNodeConstructorType(ASTNodeKind targetType) {
> >  auto const &ctors = RegistryData->nodeConstructors();
> >  auto it = llvm::find_if(
> >      ctors, [targetType](const NodeConstructorMap::value_type
&ctor) {
> >        return ctor.first.isSame(targetType);
> >      });
> >  if (it == ctors.end())
> >    return llvm::None;
> >  return it->second;
> > }
> >
> > is acceptable. The `auto it` is already acceptable, but the `auto
const& ctors` depends on an interpretation of the guideline and was rejected
in the review I submitted.
>
> This seems like a step too far from me.  I don’t know what nodeConstructors
is here at all, and you’ve erased all type information about what you’re dealing
with.
+1
>
> -Chris
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On Wed, Nov 28, 2018 at 6:25 PM Chris Lattner via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> Generally no IMO, because the cases that produce optional are not obvious.
>
Just to say, +1 from me too.

>
> > * Can we use auto in c++14 lambda arguments with llvm::find_if(C,
> [](const auto& i) { ... }) for example?
> > * We need to use auto for structured bindings, if those are used.
>
> These both get into “yes, if the type is contextually obvious”.  I’m not
> sure how to specify this though.
>
> Perhaps the rule came be rewritten more generally, as in “auto should
> generally only be used when the inferred type is obvious from context, e.g.
> as a result of a cast like dyn_cast or as a result of constructing a value
> with a constructor containing a type name.”?
>
Strongly agree.

I understand it may not be as precise and unambiguous as people would like,
I still think this is the correct core rule: there needs to be *some*
reason why the type is contextually obvious.

And honestly, I'm very happy erring on the side of writing out the type
name. I think people are too eager to use `auto` because it is easy to
write but it makes the types substantially harder for the reader to
understand.
> In the case that came up in review for me, the code I submitted is
> >
> > template <typename BaseT, typename DerivedT>
> > void registerIfNodeMatcher(...) {
> >  auto NodeKind = ASTNodeKind::getFromNodeKind<DerivedT>();
> >  if (!NodeKind.isNone())
> >    NodeCtors[NodeKind] = std::make_pair(MatcherName, Descriptor);
> > }
> >
> > but it was rejected as unreadable and I was given a link to the coding
> guidelines.
>
> I agree, this seems overly strict.
>
I mean maybe. But looking at the example, I don't have a clue what type
`NodeKind` is. I think this is borderline, and I think its fine to be
somewhat subjective based on the reviewer that is more familiar with the
code and APIs in question.

> > I'd also like to update them so that
> >
> > llvm::Optional<std::pair<std::string, MatcherCtor>>
> > getNodeConstructorType(ASTNodeKind targetType) {
> >  auto const &ctors = RegistryData->nodeConstructors();
> >  auto it = llvm::find_if(
> >      ctors, [targetType](const NodeConstructorMap::value_type
&ctor) {
> >        return ctor.first.isSame(targetType);
> >      });
> >  if (it == ctors.end())
> >    return llvm::None;
> >  return it->second;
> > }
> >
> > is acceptable. The `auto it` is already acceptable, but the `auto
const&
> ctors` depends on an interpretation of the guideline and was rejected in
> the review I submitted.
>
> This seems like a step too far from me.  I don’t know what
> nodeConstructors is here at all, and you’ve erased all type information
> about what you’re dealing with.
>
Strongly agree. The `auto it` continues to seem fine, but the `ctors` here
seems super confusing to leave off all type information.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20181204/19f8e616/attachment.html>

On 25/11/2018 14:43, Stephen Kelly via llvm-dev wrote:
> 
> However this is a proposal for more modern thinking regarding the 
> permissiveness of auto in LLVM codebases.
> 
> Currently the rule on the use of auto is here:

Hi,

Thanks for the input on this topic, which has appeared here on the 
mailing list, on the phab review, and on IRC.

Almost all respondants generally expressed the claim "The type must be 
obvious if auto is used", though as I wrote before the guide uses auto 
in context that the type is not obvious:

  for (const auto &Val : Container) { observe(Val); }

It seems that the respondants wish for 'almost never auto'. Fair enough,
if the existing practice supports that.

There is one problem though, or rather ~56,000 problems. That's how many 
uses of auto there are in the entire codebase currently, according to 
someone on IRC.

I tried to get some rough idea of how it is used. Here are the uses in 
the lib folder of clang, which hopefully rules out 'generic' code in 
headers and use in tests etc.

  $ git grep "\bauto\b" lib/ | wc -l
  12104

about half of those have an equals and angle brackets on the other side 
(dyn_cast<T> etc):

  $ git grep -e "\bauto\b" --and --not -e "auto.* =
.*<.*>" lib | wc -l
  7687

Then we can filter out the ones where it is used in for and range-for:

  $ git grep -e "\bauto\b" --and --not -e "auto.* =
.*<.*>" --and --not
-e "\bfor\b" lib | wc -l
  5007

about 2600 uses of `for (auto ...)`.

And try to eliminate uses where the result is an iterator created with 
begin/end etc:

  $ git grep -e "\bauto\b" --and --not -e "auto.* =
.*<.*>" --and --not
-e "\bfor\b" --and --not -e begin --and --not -e end --and --not -e
next
lib | wc -l
  4570

So that's 4500ish uses which remain after all of those filters.

Here is the same command run in my llvm clone:

  $ git grep -e "\bauto\b" --and --not -e "auto.* =
.*<.*>" --and --not
-e "\bfor\b" --and --not -e begin --and --not -e end --and --not -e
next
lib | wc -l
  8243

So, about 12000 uses in llvm+clang repos.

Here is a random selection of 15 after those filters in the clang repo:

git grep -e "\bauto\b" --and --not -e "auto.* =
.*<.*>" --and --not -e
"\bfor\b" --and --not -e begin --and --not -e end --and --not -e next 
lib | shuf -n 15
lib/Frontend/FrontendActions.cpp:  auto Buffer = 
FileMgr.getBufferForFile(getCurrentFile());
lib/Sema/SemaDecl.cpp:        auto IFace = MD->getClassInterface();
lib/Format/Format.cpp:  auto NewCode = applyAllReplacements(Code, Replaces);
lib/Parse/ParsePragma.cpp:  auto &Opt = Actions.getOpenCLOptions();
lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp:  auto 
CppSuspiciousNumberObjectExprM lib/Lex/HeaderSearch.cpp:    auto *HFI =
getExistingFileInfo(FE);
lib/Driver/ToolChains/Myriad.cpp:  const auto &TC
lib/CodeGen/SwiftCallingConv.cpp:      if (auto commonTy =
getCommonType(firstVecTy->getElementType(),
lib/AST/ASTDiagnostic.cpp:      if (auto nullability = 
AttributedType::stripOuterNullability(SugarRT)) {
lib/Sema/SemaStmtAttr.cpp:  auto *FnScope = S.getCurFunction();
lib/CodeGen/CGBlocks.cpp:          auto addr = 
LocalDeclMap.find(variable)->second;
lib/Serialization/ASTReaderDecl.cpp:  auto V = Record.readInt();
lib/Sema/SemaType.cpp:    // C++11 [dcl.spec.auto]p2: 'auto' is always 
fine if the declarator
lib/CodeGen/CGCall.cpp:        auto SrcLayout = 
CGM.getDataLayout().getStructLayout(STy);
lib/CodeGen/CGDeclCXX.cpp:    auto NL = 
ApplyDebugLocation::CreateEmpty(*this);


Do those uses conform to the guide? If they don't, then should the guide 
be updated? Are the types there 'obvious'?

Would the people who reject auto unless 'the type is obvious' accept all
of the ~12000 uses that pass those filters?

How did all of those uses get into the codebase? Does it indicate that 
the guide is not followed, or does it indicate that the guide is too 
subjective, or that maybe the 'the type must be obvios' guide does not 
reflect the 'reality at the coalface' anymore? Should those uses of auto
be changed?

How is a new contributor to react to any of that? What are the real 
criteria that we can use to determine where auto will cause a patch to 
be rejected? Does it only depend on who you get as a reviewer?

Here is a quote from this thread from Chris and supported by Chandler 
and Quentin at least:

 > Perhaps the rule came be rewritten more generally, as
 > in “auto should generally only be used when the inferred
 > type is obvious from context, e.g. as a result of a cast
 > like dyn_cast or as a result of constructing a value with
 > a constructor containing a type name.”?

Is it reasonable to have that as a rule if there are ~12000 uses which 
break that rule?

Here is some code from SourceManager.cpp:


  unsigned LineTableInfo::getLineTableFilenameID(StringRef Name) {
    auto IterBool = FilenameIDs.try_emplace(Name, FilenamesByID.size());
    if (IterBool.second)
      FilenamesByID.push_back(&*IterBool.first);
    return IterBool.first->second;
  }

What exactly is the type of FilenameIDs? Does knowing or not knowing 
that affect readability or maintainability? What is the exact type of 
IterBool? Does knowing or not knowing that affect readability or 
maintainability?

That code is very similar categorically to the code I submitted for 
review and which was rejected:

  llvm::Optional<std::pair<std::string, MatcherCtor>>
  getNodeConstructorType(ASTNodeKind targetType) {
    const auto &ctors = RegistryData->nodeConstructors();
    auto it = llvm::find_if(
        ctors, [targetType](const NodeConstructorMap::value_type &ctor) {
          return ctor.first.isSame(targetType);
        });
    if (it == ctors.end())
      return llvm::None;
    return it->second;
  }



Does 'ctors' being 'auto' make the code unreadable or
unmaintainable?
What if I instead write ad the review requests:

   const NodeConstructorMap &ctors = RegistryData->nodeConstructors();

Do you now understand something more than before? The type is still not 
obvious. NodeConstructorMap is just a typedef.

Both snippets are

* short
* algorithmic
* Require the local variable only for a trivial condition

and they should both be accepted or rejected, depending on whether that 
matters.

Those criteria are also the same for the other snippet that I posted before:

  template <typename BaseT, typename DerivedT>
  void registerIfNodeMatcher(...) {
    auto NodeKind = ASTNodeKind::getFromNodeKind<DerivedT>();
    if (!NodeKind.isNone())
      NodeCtors[NodeKind] = std::make_pair(MatcherName, Descriptor);
  }

[an update on this one: I changed it to

  template <typename BaseT, typename DerivedT>
  void registerIfNodeMatcher(...) {
    registerIfNodeMatcher(
        ASTNodeKind::getFromNodeKind<DerivedT>(),
        Descriptor, MatcherName);
  }

and it passed review. The return type of

  ASTNodeKind::getFromNodeKind<DerivedT>()

is no more obvious than before and now we don't have a variable name 
giving more info, so I have have further doubts that 'the type must be 
obvious' is a useful guide.  It seems instead that people reject code 
because they don't want to see 'auto'.]

Again, it is only reasonable that all three of these should be accepted 
or rejected independent of the reviewer/contributor. If that 
acceptance/rejection is dependent on the reviewer, then you have a bad 
situation.

Here is a recent commit which replaces a local typedef with auto:

  https://github.com/llvm-mirror/clang/commit/a7d1b31c

How did that get into the repo, given the interpretation of the guide in 
this thread? It appears that there are many interpretations of the guide.

If the rule is that 'the type must be obvious', as is claimed, then the 
rules are not being followed. New commits are introducing auto in 
categories where my use was rejected, and there are ~12000 uses which 
bypass some claimed 'acceptable auto' filters.

 From the perspective of a relatively new contributor, it looks like 
patches are accepted or rejected based on the use of `auto` depending on 
who is doing it, and who is reviewing.

As a new contributor, I want to know that if I have to follow a rule (or 
indeed follow the opinion of whoever is reviewing), then everyone has to 
follow the same rule. I want to know that it doesn't depend on who is 
reviewing. That doesn't currently look like the case, but maybe I'm 
missing something.

How is a new contributor to react to any of that? Is it ok that 
acceptance/rejection of a patch that uses auto depends on the reviewer? 
Especially for a contributor who sees auto finding more uses in modern 
c++ and in the ether of internet code.

Some people (At least Chris Lattner) have expressed interest in updating 
the guidelines. To what I'm not sure, but it would be good to have 
guidelines which reflect existing ground truth - even if you don't like 
that truth.

I also recommend that if someone tries to update the guidelines, they 
try to find out why the uses of `auto` which I described in this mail 
exist. It's really not 'in the interest of writability' and
'readability
doesn't matter'. I find it a bit condescending to suggest that is the 
case. I suspect that in many cases, 'the precise type is not relevant 
here' could be part of the reason those 12000 uses of auto were written. 
Maybe some research (inside and outside of llvm) would lead to insights.

Thanks,

Stephen.

Stephen Kelly via llvm-dev <llvm-dev at lists.llvm.org> writes:
> From the perspective of a relatively new contributor, it looks like
> patches are accepted or rejected based on the use of `auto` depending
> on who is doing it, and who is reviewing.
For better or for worse, that's just kind of the way things are.  Code
reviewers are human and different humans are going to have different
opinions.  I certainly have had suggestions made in the past that I
disagreed with but in the end we just have to try to understand each
other.

That said, I think it is a fool's errand to try to codify everything.
LLVM overall has done a pretty good job of providing some structure
through its coding standards while retaining flexibility for discretion.
I personally am of the opinion that "auto" is a great help and we
could
use more of it but I also understand that not everyone agrees.  It seems
impossible to me to have a prescriptive rule about when "auto" is
acceptable.  There will always be exceptions.  If one can argue the
technical merits of one's patches, it can go a long way toward educating
all of us.  :)

I tend to be more in the camp of, "no rules and let reviewers decide,"
while others prefer to have more knowledge of what will be acceptable
before they submit patches.  I've always bristled at rules like, "use
C++14 except this and that feature."  If we're using C++ X, let's
use
C++ X.  Again, I think LLVM strikes a pretty good balance overall.

                            -David

On Dec 16, 2018, at 11:44 AM, Stephen Kelly via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> On 25/11/2018 14:43, Stephen Kelly via llvm-dev wrote:
>> However this is a proposal for more modern thinking regarding the
permissiveness of auto in LLVM codebases.
>> Currently the rule on the use of auto is here:
> 
> Hi,
> 
> Thanks for the input on this topic, which has appeared here on the mailing
list, on the phab review, and on IRC.
> 
> Almost all respondants generally expressed the claim "The type must be
obvious if auto is used", though as I wrote before the guide uses auto in
context that the type is not obvious:
> 
> for (const auto &Val : Container) { observe(Val); }
> 
> It seems that the respondants wish for 'almost never auto'. Fair
enough, if the existing practice supports that.
> 
> There is one problem though, or rather ~56,000 problems. That's how
many uses of auto there are in the entire codebase currently, according to
someone on IRC.
I find this to be a helpful line of argument.  We should, as a community, decide
what the right thing is regardless of the existing art.  As you say, the current
patches going in have been arbitrary, so making an argument based on what is in
the code base isn’t particularly informative.
> Do those uses conform to the guide? If they don't, then should the
guide be updated? Are the types there 'obvious’?
If/when we revise the policy, then it would make sense for non-conforming uses
of auto to be changed.  However, I don’t think that actually making a widespread
change would be high priority...
> How did all of those uses get into the codebase? Does it indicate that the
guide is not followed, or does it indicate that the guide is too subjective, or
that maybe the 'the type must be obvios' guide does not reflect the
'reality at the coalface' anymore? Should those uses of auto be changed?
My understanding is that there has been no widely understood or accepted policy,
so different coders and reviewers are doing different things.
> How is a new contributor to react to any of that? What are the real
criteria that we can use to determine where auto will cause a patch to be
rejected? Does it only depend on who you get as a reviewer?
Right now, it is quite arbitrary.  This is a bug, not a feature.
> Here is a quote from this thread from Chris and supported by Chandler and
Quentin at least:
> 
> > Perhaps the rule came be rewritten more generally, as
> > in “auto should generally only be used when the inferred
> > type is obvious from context, e.g. as a result of a cast
> > like dyn_cast or as a result of constructing a value with
> > a constructor containing a type name.”?
> 
> Is it reasonable to have that as a rule if there are ~12000 uses which
break that rule?
Yes.  If you’d like to make progress on this, I think you should start by
building consensus.  It seems like there is widespread enough acknowledgement
that the current state of things is broken, but there is no concrete proposal
for a coding standards change.  Please prepare a patch so we can discuss it.

-Chris