[llvm-dev] RFC: Modernizing our use of auto

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[llvm-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev

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.


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev
On Nov 25, 2018, at 6:43 AM, Stephen Kelly via llvm-dev <[hidden email]> 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
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev
Le mer. 28 nov. 2018 à 09:26, Chris Lattner via llvm-dev
<[hidden email]> a écrit :

>
> On Nov 25, 2018, at 6:43 AM, Stephen Kelly via llvm-dev <[hidden email]> 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
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [cfe-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev
In reply to this post by Chris Lattner via llvm-dev
On Wed, Nov 28, 2018 at 6:25 PM Chris Lattner via cfe-dev <[hidden email]> 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.

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [cfe-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev

> > Perhaps the rule came be rewritten more generally, as in “auto should generally only be

> > used when the inferred type is obvious from context,

 

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

 

FWIW, auto != infer. Since AAA* was mentioned and I wrote that, I just wanted to clarify never intended it to mean to always use auto for type inference.

 

I’ve always tried to carefully say there are two forms, but people seem to fixate on the first one:

 

              auto x = non-cast-expr; // inferred type

 

              auto x = type( expr );      // explicit type – includes dynamic_cast<>, {} vs () syntax, etc.

 

The advice to “always auto” includes the second form, and so you can always use auto without loss of readability because it’s not in tension with stating an explicit type. FAQ: Why write the second form “auto x = type( expr );” instead of just the traditional “type x( expr );”? For several reasons, perhaps the most important being that with “auto” you can never forget to initialize a variable because the = is mandatory. There are also other advantages, such as no vexing parse, consistency with other left-to-right modern declarations such as using, and others; all these advantages apply to both forms above, you get them just by starting with “auto.”

 

But again this is just a “FWIW,” I’m just clarifying, not trying to campaign. The main thing for any team is to adopt a style that people are happy to live with.

 

Herb

 

 

* Actually AA these days. Thanks, Richard, both for promoting guaranteed copy elision and for pointing out that it eliminates the first A! Auto style is now easy and efficient to use with all types, even std::array and lock_guard. Insert gratuitous battery size joke here about how AA is an upgrade, carries more charge, etc.

 

 

 

From: cfe-dev <[hidden email]> On Behalf Of Chandler Carruth via cfe-dev
Sent: Tuesday, December 4, 2018 5:02 AM
To: Chris Lattner <[hidden email]>
Cc: [hidden email]; [hidden email]; Stephen Kelly <[hidden email]>
Subject: Re: [cfe-dev] [llvm-dev] RFC: Modernizing our use of auto

 

On Wed, Nov 28, 2018 at 6:25 PM Chris Lattner via cfe-dev <[hidden email]> 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.


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [cfe-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev
In reply to this post by Chris Lattner via llvm-dev
> 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

I'm probably the Nth person to ask this, but what keeps us from promoting the use of a clang-tidy-powered tool that basically emits fixits of s/auto/actual_type/?

It sounds like we're pretty constrained in where we can use auto (which I'm completely in favor of, in no small part because of our pervasive use of types spelled *Ref); a tool that's able to do that replacement and correctly ignore trivially-okay `auto`s >90% of the time sounds like it'd make both sides of this:

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

happy.

Naturally, this gets tricky as we move to c++ versions that allow auto in more places, and is iffy in general in e.g. type-dependent contexts, but I didn't say 100% accuracy ;)

On Tue, Dec 4, 2018 at 2:02 AM Chandler Carruth via llvm-dev <[hidden email]> wrote:
On Wed, Nov 28, 2018 at 6:25 PM Chris Lattner via cfe-dev <[hidden email]> 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.
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [cfe-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev
In reply to this post by Chris Lattner via llvm-dev
I pretty much agree with this.  However, I'll mention that the contentious use case in the code review came from something that is technically the second form, the problem is just that it wasn't obvious.  Specifically, the line was this:

auto NodeKind = ASTNodeKind::getFromNodeKind<DerivedT>();

Technically this is no different than saying 

auto file = File::open(open_flags);

The return type is File, so it should fall under always auto.  The objection that arose was with the ASTNodeKind line that it wasn't obvious to someone unfamiliar with that code that it was such a function.  For starters, one might think the return type is something like ASTNode, not ASTNodeKind.  Additionally, because the factory function takes a template argument, it's not clear if the return type also depends on that template argument.  Is it an ASTNode<DerivedT>, or ASTNodeKind<DerivedT>, or maybe just a unique_ptr<DerivedT>.  

So at the risk of stating the obvious -- just having the return type spelled out on the RHS of the equal sign is not sufficient, it must also be obvious that the return type is spelled out on the RHS of the equal sign.  With something like dynamic_cast<T>, etc it is, because these are well-established language constructs.  But with user-defined types, not always.

So this is where I agree with Chandler, that a case like this is borderline and there's some judgement on the part of the person looking at the code review.

On Tue, Dec 4, 2018 at 11:22 AM Herb Sutter via llvm-dev <[hidden email]> wrote:

> > Perhaps the rule came be rewritten more generally, as in “auto should generally only be

> > used when the inferred type is obvious from context,

 

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

 

FWIW, auto != infer. Since AAA* was mentioned and I wrote that, I just wanted to clarify never intended it to mean to always use auto for type inference.

 

I’ve always tried to carefully say there are two forms, but people seem to fixate on the first one:

 

              auto x = non-cast-expr; // inferred type

 

              auto x = type( expr );      // explicit type – includes dynamic_cast<>, {} vs () syntax, etc.

 

The advice to “always auto” includes the second form, and so you can always use auto without loss of readability because it’s not in tension with stating an explicit type. FAQ: Why write the second form “auto x = type( expr );” instead of just the traditional “type x( expr );”? For several reasons, perhaps the most important being that with “auto” you can never forget to initialize a variable because the = is mandatory. There are also other advantages, such as no vexing parse, consistency with other left-to-right modern declarations such as using, and others; all these advantages apply to both forms above, you get them just by starting with “auto.”

 

But again this is just a “FWIW,” I’m just clarifying, not trying to campaign. The main thing for any team is to adopt a style that people are happy to live with.

 

Herb

 

 

* Actually AA these days. Thanks, Richard, both for promoting guaranteed copy elision and for pointing out that it eliminates the first A! Auto style is now easy and efficient to use with all types, even std::array and lock_guard. Insert gratuitous battery size joke here about how AA is an upgrade, carries more charge, etc.

 

 

 

From: cfe-dev <[hidden email]> On Behalf Of Chandler Carruth via cfe-dev
Sent: Tuesday, December 4, 2018 5:02 AM
To: Chris Lattner <[hidden email]>
Cc: [hidden email]; [hidden email]; Stephen Kelly <[hidden email]>
Subject: Re: [cfe-dev] [llvm-dev] RFC: Modernizing our use of auto

 

On Wed, Nov 28, 2018 at 6:25 PM Chris Lattner via cfe-dev <[hidden email]> 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.

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [cfe-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev
In reply to this post by Chris Lattner via llvm-dev


> On Dec 4, 2018, at 10:59 AM, George Burgess IV <[hidden email]> wrote:
>
> > 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
>
> I'm probably the Nth person to ask this, but what keeps us from promoting the use of a clang-tidy-powered tool that basically emits fixits of s/auto/actual_type/?

Because the tool would need to apply judgement to when this makes sense.  If we can’t write an algorithm in coding standards.html to be prescriptive about when to use auto, then I don’t think we can automate this.

-Chris


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [cfe-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev
So the problem becomes one of seeing if people will accept `auto` rules that don't require substantial amounts of thought.

Personally, I see this like our use of clang-format. It might not use my favorite color of paint, but it's uniform, automatic, and it lets me entirely forget about tons of style nits, so I love it to death. Hence, if we need to start with "no auto, except in this small set of trivially OK and machine-verifiable cases, which we'll consider expanding this as need arises," to get to that, I'd be all for it.

I realize that many devs probably strongly disagree with me here, but that's my 2c.

On Wed, Dec 5, 2018 at 9:26 PM Chris Lattner <[hidden email]> wrote:


> On Dec 4, 2018, at 10:59 AM, George Burgess IV <[hidden email]> wrote:
>
> > 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
>
> I'm probably the Nth person to ask this, but what keeps us from promoting the use of a clang-tidy-powered tool that basically emits fixits of s/auto/actual_type/?

Because the tool would need to apply judgement to when this makes sense.  If we can’t write an algorithm in coding standards.html to be prescriptive about when to use auto, then I don’t think we can automate this.

-Chris



_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [cfe-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev
I agree with George here, the current implementation of modernize-use-auto uses pretty much the same rules as described in our coding standards which we all agree on, mainly:
- iterators (begin, end)
- cast expressions
- new expression

Ofc there are more cases where type is obvious and where we would like to use auto, but automating the part that modernize-use-auto can do is enough to cover most of the cases.

Cheers
Piotr

czw., 6 gru 2018 o 08:46 George Burgess IV via cfe-dev <[hidden email]> napisał(a):
So the problem becomes one of seeing if people will accept `auto` rules that don't require substantial amounts of thought.

Personally, I see this like our use of clang-format. It might not use my favorite color of paint, but it's uniform, automatic, and it lets me entirely forget about tons of style nits, so I love it to death. Hence, if we need to start with "no auto, except in this small set of trivially OK and machine-verifiable cases, which we'll consider expanding this as need arises," to get to that, I'd be all for it.

I realize that many devs probably strongly disagree with me here, but that's my 2c.

On Wed, Dec 5, 2018 at 9:26 PM Chris Lattner <[hidden email]> wrote:


> On Dec 4, 2018, at 10:59 AM, George Burgess IV <[hidden email]> wrote:
>
> > 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
>
> I'm probably the Nth person to ask this, but what keeps us from promoting the use of a clang-tidy-powered tool that basically emits fixits of s/auto/actual_type/?

Because the tool would need to apply judgement to when this makes sense.  If we can’t write an algorithm in coding standards.html to be prescriptive about when to use auto, then I don’t think we can automate this.

-Chris


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [cfe-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev
In reply to this post by Chris Lattner via llvm-dev


On Wed, Dec 5, 2018, 9:27 PM Chris Lattner via cfe-dev <[hidden email] wrote:


> On Dec 4, 2018, at 10:59 AM, George Burgess IV <[hidden email]> wrote:
>
> > 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
>
> I'm probably the Nth person to ask this, but what keeps us from promoting the use of a clang-tidy-powered tool that basically emits fixits of s/auto/actual_type/?

For me: I still don't know how to integrate clang-tidy into my development workflow. I'm open to pointers to documentation/etc.

Because the tool would need to apply judgement to when this makes sense.  If we can’t write an algorithm in coding standards.html to be prescriptive about when to use auto, then I don’t think we can automate this.

Yeah, I don't think we can automate it entirely, but at least it might lower the writing cost to make it easier for folks favoring auto for writability where community standards would prefer the named type. Nice thing about clang-tidy etc is if integrated well, it should only flag on code in the current diff, and only as a suppressible suggestion not a hard requirement.

I think if folks want to work on that, it'd be appreciated, but I don't think it changes the discussion around what style we want.




-Chris


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [cfe-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev
In reply to this post by Chris Lattner via llvm-dev
I think modernize-use-auto is good for the problem we aren't discussion and maybe a problem we don't have (using auto not enough and wanting to migrate code to use it more). A tool that gives a "hey, maybe your use of auto here is more for writability than readability" would be a different thing, and probably a bit challenging to tune it's false positive rate to not be annoying etc.

On Thu, Dec 6, 2018, 2:13 AM Piotr Padlewski via llvm-dev <[hidden email] wrote:
I agree with George here, the current implementation of modernize-use-auto uses pretty much the same rules as described in our coding standards which we all agree on, mainly:
- iterators (begin, end)
- cast expressions
- new expression

Ofc there are more cases where type is obvious and where we would like to use auto, but automating the part that modernize-use-auto can do is enough to cover most of the cases.

Cheers
Piotr

czw., 6 gru 2018 o 08:46 George Burgess IV via cfe-dev <[hidden email]> napisał(a):
So the problem becomes one of seeing if people will accept `auto` rules that don't require substantial amounts of thought.

Personally, I see this like our use of clang-format. It might not use my favorite color of paint, but it's uniform, automatic, and it lets me entirely forget about tons of style nits, so I love it to death. Hence, if we need to start with "no auto, except in this small set of trivially OK and machine-verifiable cases, which we'll consider expanding this as need arises," to get to that, I'd be all for it.

I realize that many devs probably strongly disagree with me here, but that's my 2c.

On Wed, Dec 5, 2018 at 9:26 PM Chris Lattner <[hidden email]> wrote:


> On Dec 4, 2018, at 10:59 AM, George Burgess IV <[hidden email]> wrote:
>
> > 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
>
> I'm probably the Nth person to ask this, but what keeps us from promoting the use of a clang-tidy-powered tool that basically emits fixits of s/auto/actual_type/?

Because the tool would need to apply judgement to when this makes sense.  If we can’t write an algorithm in coding standards.html to be prescriptive about when to use auto, then I don’t think we can automate this.

-Chris


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [cfe-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev
In reply to this post by Chris Lattner via llvm-dev


On Dec 6, 2018, at 11:37 AM, David Blaikie via llvm-dev <[hidden email]> wrote:



On Wed, Dec 5, 2018, 9:27 PM Chris Lattner via cfe-dev <[hidden email] wrote:


> On Dec 4, 2018, at 10:59 AM, George Burgess IV <[hidden email]> wrote:
>
> > 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
>
> I'm probably the Nth person to ask this, but what keeps us from promoting the use of a clang-tidy-powered tool that basically emits fixits of s/auto/actual_type/?

For me: I still don't know how to integrate clang-tidy into my development workflow. I'm open to pointers to documentation/etc.

Not sure if this helps, but CMake actually added support for integrating clang-tidy with your build:

I haven’t used it with LLVM because LLVM and Clang aren’t clang-tidy clean, but I do use it on other projects that are, and it works really well.

-Chris


Because the tool would need to apply judgement to when this makes sense.  If we can’t write an algorithm in coding standards.html to be prescriptive about when to use auto, then I don’t think we can automate this.

Yeah, I don't think we can automate it entirely, but at least it might lower the writing cost to make it easier for folks favoring auto for writability where community standards would prefer the named type. Nice thing about clang-tidy etc is if integrated well, it should only flag on code in the current diff, and only as a suppressible suggestion not a hard requirement.

I think if folks want to work on that, it'd be appreciated, but I don't think it changes the discussion around what style we want.




-Chris


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev
In reply to this post by Chris Lattner via llvm-dev
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.

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev
Stephen Kelly via llvm-dev <[hidden email]> 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
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev
In reply to this post by Chris Lattner via llvm-dev
On Dec 16, 2018, at 11:44 AM, Stephen Kelly via llvm-dev <[hidden email]> 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


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev


On 31/12/2018 04:54, Chris Lattner wrote:
On Dec 16, 2018, at 11:44 AM, Stephen Kelly via llvm-dev [hidden email] 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.


Given what you wrote below, maybe you are missing a negation somewhere in this sentence?


We should, as a community, decide what the right thing is regardless of the existing art.


The existing art is part of 'the community deciding what to do'.


And yes, I think it makes sense to 'standardize existing practice' where possible.


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.


And different contexts within LLVM are fine with auto, but seem to get campaigning from other parts who have a different interpretation of the guideline:


https://reviews.llvm.org/D33672#inline-475812


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.


Do we have some idea of who is interested in fixing the bug? It can't be just one person fixing it - this is a community issue. You've suggested that the guideline needs an update, and I've already suggested an update. Is it only the two of us? How can we proceed?


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?

    
If you’d like to make progress on this, I think you should start by building consensus.


Well, I'm trying to find out what the positions people have are, but even though there are so many existing usages of auto, this thread is not getting responses from the people who put them there. So, the code says one thing, and the guideline says arguably the same thing, but people have alternative interpretations of the guideline. But at least the responses here are not really representative.

For me that means I'm not able to get my clang-query features (https://steveire.wordpress.com/2018/11/11/future-developments-in-clang-query/) upstream because I'm getting reviews which say "remove auto, here and everywhere in this file." in


 https://reviews.llvm.org/D54408?id=173770#inline-480255


That's a bit of a difficult review comment, given the ways it is already used throughout the code.


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.


I made a proposal in my initial mail in this thread. See the end of the email: https://lists.llvm.org/pipermail/llvm-dev/2018-November/127953.html

Phab is not well-suited to discussion like this, so we should probably keep it on the mailing list for now.


But, here's some updated thinking:


New guidelines should

* Be easy to follow
* Have some consistency
* Be modern
* Be welcoming (or at least non-hostile) to newcomers
* Standardize existing practice in LLVM
* Achieve a consensus of support about the spirit of the guideline (consensus is not unanimity) and be acceptable to people who dislike auto

Can we agree on that much to start?


On the Phab review, some people expressed that they liked the examples of when auto is acceptable. Here is an updated attempt at guideline text for that section:



```
Some are advocating a policy of "almost always ``auto``" in C++11, however LLVM
uses a more moderate stance. Don't "almost always" use ``auto``, but it may be used where either the Concept or the type is obvious from the context. Here are
some cases where use of ``auto`` would make sense:

* Where the type appears elsewhere in the line (eg a dyn_cast)
* Where the variable name or initializing expression provides enough information (`auto SR = getSourceRange()`)
* Where the context makes the *Concept* obvious, even if the type is not obvious (eg, where the instance is only used as an iterator, or in an algorithm as a container-like concept, or only with a validity check, or an AST Matcher).

Exceptions may arise, but they should only arise in exceptional cases. If the case is not exceptional, apply the guidelines in review discussion.
```


The most important thing here is that it does not accept your proposal that 'the type must be obvious'. Instead, it recognizes that `auto` is really "an unspecified concept" - unspecified only because we can't specify the concept in C++ yet.

However, the point/concern seems to be that readers of code should know the instance may be used in its local scope.

That's why these guidelines would allow `auto Ctors` in


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;
 }
 

because `Ctors` is obviously the Container *concept*, and knowing exactly what type it is is not necessary in the local context.

However, in the below code it would probably not be ok because we're calling methods on the instance which opens up more possibilities (is it a base interface? etc):


void SomeClass::foo(int input)
{
    auto Ctors = getCtors(input);

    m_widgets = Ctors->calculate();
}

Here, `Ctors` is definitely not a Container concept, we don't know what kind of concept it is, so we should know the type by seeing it typed in the code.

Another example from earlier in the thread:


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

Here, `NodeKind` is used as an Optional (or Maybe) *concept*. All we do is a validity check. So, `auto` should be allowed here.

This 'the concept must be obvious' guideline is also what allows the use of `auto` for iterators.


What do people think of "Either the Concept or the type should be obvious from the context" as a baseline guideline?


Thanks,

Stephen.



_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [cfe-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev


On Sat, Jan 5, 2019 at 11:37 AM Stephen Kelly via cfe-dev <[hidden email]> wrote:


On 31/12/2018 04:54, Chris Lattner wrote:
On Dec 16, 2018, at 11:44 AM, Stephen Kelly via llvm-dev [hidden email] 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.


Given what you wrote below, maybe you are missing a negation somewhere in this sentence?


We should, as a community, decide what the right thing is regardless of the existing art.


The existing art is part of 'the community deciding what to do'.


And yes, I think it makes sense to 'standardize existing practice' where possible.


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.


And different contexts within LLVM are fine with auto, but seem to get campaigning from other parts who have a different interpretation of the guideline:


https://reviews.llvm.org/D33672#inline-475812


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.


Do we have some idea of who is interested in fixing the bug? It can't be just one person fixing it - this is a community issue. You've suggested that the guideline needs an update, and I've already suggested an update. Is it only the two of us? How can we proceed?


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?

    
If you’d like to make progress on this, I think you should start by building consensus.


Well, I'm trying to find out what the positions people have are, but even though there are so many existing usages of auto, this thread is not getting responses from the people who put them there. So, the code says one thing, and the guideline says arguably the same thing, but people have alternative interpretations of the guideline. But at least the responses here are not really representative.


You're discounting some aspects:

- Code is written downstream without much consideration for the guideline and then upstreamed
- Review process isn't perfect: we let things slip, we don't always want to nitpick on things like `auto`, etc.
- Some developers just don't know the guidelines and because of the point above, some cases slip.
- People move back and forth between projects (i.e. LLVM is not their main project) and thus aren't focused on every details of the code guidelines (For instance I would review a patch today and not necessarily catch on every style aspect).

For the particular case of auto, there is an even more complex effect: since the intention is that "types should be obvious when reading the code", someone very familiar with some libraries will always unconsciously infer the returned type from API calls (from conventions, past experience, etc.), but someone who is less familiar with this area of the codebase would be quickly confused.
Now add to this the fact that the reviewers are (in general) familiar with the area they are reviewing, they aren't necessarily in a good position to catch immediately all the uses of auto that makes the types less obvious to other people.

The view you mentioned above is a good example of this: one reviewer points very accurately that "if you have to explain that in the variable's name, justify it in review comments" then "This really shouldn't be auto".
To me this is the important part of the guideline: make it easy for anyone to read the code and infer the types (ideally without extra steps like clicking in an IDE to debunk it).

On the other hand, another reviewer mentions that there is a specific pattern here and that "people get used to it very quickly when they start actively working on the codebase."
This is also a valid point, and while I'm very much in favor of the current guideline in general, there are pattern that are so much repetitive that it can be worthwhile to endorse them as the kind of things you need to know for this area.

The question in this case is if:

const auto ValueToCast = ....getAs<DefinedOrUnknownSVal>();

has an "obvious" type of being an Optional<DefinedOrUnknownSVal>.

I claim that this example is less about `auto` itself but rather a question about can you consider that `getAs` is such a "core" pattern of this area of the code base that we can accept as "common knowledge" that it always wrap the returned type in an optional.
And if we do, then the current guideline is actually fulfilled: "the type is already obvious from the context" (getAs being part of the context at this point).

There are other such example, for example I believe we can assume that all the standard STL algorithm are known and so in this case: 
 llvm::all_of(Container, [] (const auto &Value) { ...})
 
The use of auto in the lambda should be OK (assuming c++14 and the type of Container is obvious).


For me that means I'm not able to get my clang-query features (https://steveire.wordpress.com/2018/11/11/future-developments-in-clang-query/) upstream because I'm getting reviews which say "remove auto, here and everywhere in this file." in


 https://reviews.llvm.org/D54408?id=173770#inline-480255


That's a bit of a difficult review comment, given the ways it is already used throughout the code.


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.


I made a proposal in my initial mail in this thread. See the end of the email: https://lists.llvm.org/pipermail/llvm-dev/2018-November/127953.html

Phab is not well-suited to discussion like this, so we should probably keep it on the mailing list for now.


But, here's some updated thinking:


New guidelines should

* Be easy to follow
* Have some consistency
* Be modern
* Be welcoming (or at least non-hostile) to newcomers
* Standardize existing practice in LLVM
* Achieve a consensus of support about the spirit of the guideline (consensus is not unanimity) and be acceptable to people who dislike auto

Can we agree on that much to start?


It depends what you mean by "standardize existing practice in LLVM", this seems like a "guideline" to define the new guideline, more than a rule. I.e. if some area are "abusing" auto for example, this is not automatically a reason to standardize, this may just be an indication that some cleanup is needed there.

 


On the Phab review, some people expressed that they liked the examples of when auto is acceptable. Here is an updated attempt at guideline text for that section:



```
Some are advocating a policy of "almost always ``auto``" in C++11, however LLVM
uses a more moderate stance. Don't "almost always" use ``auto``, but it may be used where either the Concept or the type is obvious from the context. Here are
some cases where use of ``auto`` would make sense:

* Where the type appears elsewhere in the line (eg a dyn_cast)
* Where the variable name or initializing expression provides enough information (`auto SR = getSourceRange()`)
* Where the context makes the *Concept* obvious, even if the type is not obvious (eg, where the instance is only used as an iterator, or in an algorithm as a container-like concept, or only with a validity check, or an AST Matcher).


The later point isn't clear to me: even if you're only using the instance as an iterator, I may want to know what types the iterator is actually iterating on.
I'm not saying that the idea you have here is not desirable, just that the language used does not help me visualize what is / isn't OK: it does not fit your first criteria "Be easy to follow".
 


Exceptions may arise, but they should only arise in exceptional cases. If the case is not exceptional, apply the guidelines in review discussion.


This last sentence seems general to the full document rather than this section?
 

```


The most important thing here is that it does not accept your proposal that 'the type must be obvious'. Instead, it recognizes that `auto` is really "an unspecified concept" - unspecified only because we can't specify the concept in C++ yet.

However, the point/concern seems to be that readers of code should know the instance may be used in its local scope.

That's why these guidelines would allow `auto Ctors` in


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;
 }
 

because `Ctors` is obviously the Container *concept*

It is only obvious after your read the following, but more importantly: why use auto here? It wouldn't hurt to write:    const NodeConstructorMap &Ctors = RegistryData->nodeConstructors(); 
I.e. auto does not make the code more readable to me in this case.
On the other hand, if the type of Ctors is explicit, the lambda argument type could be auto to me (it becomes obvious from the local context and the use of llvm::find_if).
 

, and knowing exactly what type it is is not necessary in the local context. 

However, in the below code it would probably not be ok because we're calling methods on the instance which opens up more possibilities (is it a base interface? etc):


void SomeClass::foo(int input)
{
    auto Ctors = getCtors(input);

    m_widgets = Ctors->calculate();
}

Here, `Ctors` is definitely not a Container concept, we don't know what kind of concept it is, so we should know the type by seeing it typed in the code.

Another example from earlier in the thread:


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

Here, `NodeKind` is used as an Optional (or Maybe) *concept*. All we do is a validity check. So, `auto` should be allowed here.

This 'the concept must be obvious' guideline is also what allows the use of `auto` for iterators.


What do people think of "Either the Concept or the type should be obvious from the context" as a baseline guideline?

You forgot to add "and knowing exactly what type it is is not necessary in the local context" after "Concept", it seems that this is necessary for your definition.

I'm still fairly unconvinced, because the concept of "concept" seems too fuzzy to be applicable in such a guideline. What is a "Concept" other than a class that honor an API? How is your previous example " m_widgets = Ctors->calculate();" not just obvious that Ctors is an instance of a Concept that "can calculate a widget"?

Best,

-- 
Mehdi


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [cfe-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev


On 05/01/2019 21:56, Mehdi AMINI wrote:
You're discounting some aspects:


Not surprising! :)

Thanks for your contribution. Lots of things I hadn't considered here.



- Code is written downstream without much consideration for the guideline and then upstreamed


I hadn't considered this. Can you either quantify it, or point to some examples? I guess you're referring to some large commits which might not have been reviewed for particulars of style. Can you point me to some?


- Review process isn't perfect: we let things slip, we don't always want to nitpick on things like `auto`, etc.


Yes, even in that case, the submitter thinks the submitted code makes sense and the reviewer doesn't think it doesn't make sense. At least not so much that they'd block a commit because of it :).


- Some developers just don't know the guidelines and because of the point above, some cases slip.
- People move back and forth between projects (i.e. LLVM is not their main project) and thus aren't focused on every details of the code guidelines (For instance I would review a patch today and not necessarily catch on every style aspect).

For the particular case of auto, there is an even more complex effect: since the intention is that "types should be obvious when reading the code", someone very familiar with some libraries will always unconsciously infer the returned type from API calls (from conventions, past experience, etc.), but someone who is less familiar with this area of the codebase would be quickly confused.


Intermediate AST Matchers typically look like

 auto ConstrExpr = cxxConstructExpr(hasType(recordDecl(hasName(ClassName))));

`auto` is used exclusively when writing AST Matchers variables. No one complains about not reading the exact type of `ConstrExpr` in the code. And the people who work on those parts of the code tend to be the people most against all use of `auto`.


(You could write `StatementMatcher` instead of `auto` above if you wished, and `DeclarationMatcher`, `TypeMatcher` etc for other matchers. Unless you were familiar with all the implementation details of AST Matchers those type names wouldn't mean anything to you. In fact, they're not type names - they're just typedefs. Typedefs are what we used to use before we were able to use auto, when the actual type doesn't matter. You can look up the actual expansion of the typedef if you wish, but it won't make the above code clearer to you, because if you know what AST Matchers are, the exact type of `ConstrExpr` *does not matter*. Only the concept matters. It's an AST Matcher and it can be nested inside other AST Matchers and passed to Finder->addMatcher).


So, you don't see the exact type, and that doesn't affect your confusion about the above line. It could be `auto`, `StatementMatcher` or `internal::Matcher<Stmt>` without affecting your confusion. If you felt confusion about that line of code, it wasn't about the type.


I claim that this example is less about `auto` itself but rather a question about can you consider that `getAs` is such a "core" pattern of this area of the code base that we can accept as "common knowledge" that it always wrap the returned type in an optional.
And if we do, then the current guideline is actually fulfilled: "the type is already obvious from the context" (getAs being part of the context at this point).


We probably don't need to dive into the details here. You will notice though that the conclusion from the same people who discussed that revisited it in https://reviews.llvm.org/D54877#inline-484380 and concluded retrospectively that `auto` was fine.

 


There are other such example, for example I believe we can assume that all the standard STL algorithm are known and so in this case: 
 llvm::all_of(Container, [] (const auto &Value) { ...})
 
The use of auto in the lambda should be OK (assuming c++14 and the type of Container is obvious).


If you require that something about the type (either the type itself or the concept - ie being a container) or the type passed to the lambda be obvious, you can choose either one. My proposal for the LLVM guideline is not to make both the container and the lambda arg `auto`. But you could choose one or the other. I think you're agreeing with that.




For me that means I'm not able to get my clang-query features (https://steveire.wordpress.com/2018/11/11/future-developments-in-clang-query/) upstream because I'm getting reviews which say "remove auto, here and everywhere in this file." in


 https://reviews.llvm.org/D54408?id=173770#inline-480255


That's a bit of a difficult review comment, given the ways it is already used throughout the code.


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.


I made a proposal in my initial mail in this thread. See the end of the email: https://lists.llvm.org/pipermail/llvm-dev/2018-November/127953.html

Phab is not well-suited to discussion like this, so we should probably keep it on the mailing list for now.


But, here's some updated thinking:


New guidelines should

* Be easy to follow
* Have some consistency
* Be modern
* Be welcoming (or at least non-hostile) to newcomers
* Standardize existing practice in LLVM
* Achieve a consensus of support about the spirit of the guideline (consensus is not unanimity) and be acceptable to people who dislike auto

Can we agree on that much to start?


It depends what you mean by "standardize existing practice in LLVM", this seems like a "guideline" to define the new guideline, more than a rule. I.e. if some area are "abusing" auto for example, this is not automatically a reason to standardize, this may just be an indication that some cleanup is needed there.


Certainly! I tried to quantify the existing use of `auto` a bit in a previous email. Can you do a more-useful quantification?


 


On the Phab review, some people expressed that they liked the examples of when auto is acceptable. Here is an updated attempt at guideline text for that section:



```
Some are advocating a policy of "almost always ``auto``" in C++11, however LLVM
uses a more moderate stance. Don't "almost always" use ``auto``, but it may be used where either the Concept or the type is obvious from the context. Here are
some cases where use of ``auto`` would make sense:

* Where the type appears elsewhere in the line (eg a dyn_cast)
* Where the variable name or initializing expression provides enough information (`auto SR = getSourceRange()`)
* Where the context makes the *Concept* obvious, even if the type is not obvious (eg, where the instance is only used as an iterator, or in an algorithm as a container-like concept, or only with a validity check, or an AST Matcher).


The later point isn't clear to me: even if you're only using the instance as an iterator, I may want to know what types the iterator is actually iterating on.


Hmm, the existing guideline says that these iterators are ok as `auto`. I think you're saying that if there's a reason to have an exception for a particular iterator then you can have an exception for it. I agree with that! Are you thinking of some exceptional case?


I'm not saying that the idea you have here is not desirable, just that the language used does not help me visualize what is / isn't OK: it does not fit your first criteria "Be easy to follow".
 


Exceptions may arise, but they should only arise in exceptional cases. If the case is not exceptional, apply the guidelines in review discussion.


This last sentence seems general to the full document rather than this section?


Yes, I'm just trying to satisfy people who might be worried :).


 

```


The most important thing here is that it does not accept your proposal that 'the type must be obvious'. Instead, it recognizes that `auto` is really "an unspecified concept" - unspecified only because we can't specify the concept in C++ yet.

However, the point/concern seems to be that readers of code should know the instance may be used in its local scope.

That's why these guidelines would allow `auto Ctors` in


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;
 }
 

because `Ctors` is obviously the Container *concept*

It is only obvious after your read the following, but more importantly: why use auto here? It wouldn't hurt to write:    const NodeConstructorMap &Ctors = RegistryData->nodeConstructors();


Why write `NodeConstructorMap`? That's not a type. If the reason for not using `auto` is that "the type must be obvious", then surely you're obliged to expand typedefs? `NodeConstructorMap` could be a std::vector for all you know...


The only thing you care about is that it's a container. It's a short function that uses the variable with `llvm::find_if`. Both the shortness and the use with `find_if` are relevant. This thing is a container, and I can't think of a container where the precise type matters in a small algorithmic function like this.


But the *Container-ness* of it does matter. And it is obvious. That's the concept that is obvious, so it should be allowed by the guidelines and should pass review.


I.e. auto does not make the code more readable to me in this case.


`NodeConstructorMap` doesn't "make the type obvious" either.


On the other hand, if the type of Ctors is explicit, the lambda argument type could be auto to me (it becomes obvious from the local context and the use of llvm::find_if).


Fair enough - I agree that this is a consequence of the guideline I propose, as I wrote above.


 

, and knowing exactly what type it is is not necessary in the local context. 

However, in the below code it would probably not be ok because we're calling methods on the instance which opens up more possibilities (is it a base interface? etc):


void SomeClass::foo(int input)
{
    auto Ctors = getCtors(input);

    m_widgets = Ctors->calculate();
}

Here, `Ctors` is definitely not a Container concept, we don't know what kind of concept it is, so we should know the type by seeing it typed in the code.

Another example from earlier in the thread:


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

Here, `NodeKind` is used as an Optional (or Maybe) *concept*. All we do is a validity check. So, `auto` should be allowed here.

This 'the concept must be obvious' guideline is also what allows the use of `auto` for iterators.


What do people think of "Either the Concept or the type should be obvious from the context" as a baseline guideline?

You forgot to add "and knowing exactly what type it is is not necessary in the local context" after "Concept", it seems that this is necessary for your definition.


Correct! Thanks for the clarification. I mentioned earlier in the thread various things about use of `auto` being ok where the type is irrelevant, but indeed I didn't put that in the proposed guideline and your clarification is useful.



I'm still fairly unconvinced, because the concept of "concept" seems too fuzzy to be applicable in such a guideline. What is a "Concept" other than a class that honor an API? How is your previous example " m_widgets = Ctors->calculate();" not just obvious that Ctors is an instance of a Concept that "can calculate a widget"?


Indeed, but if you go down that path, you arrive at AAA, and I don't want to propose that. I said that in the first line of this thread, but expressing the reason for that in terms of Concepts clarifies the reason for that in my mind. So, thanks for the discussion!


There are more general concepts, such as Iterator (ie if you see this being used in a raw for loop or to compare with the result of an algorithm it is not surprising), Container/Range (ie, if this is passed directly to a range-capable algorithm such as find_if, it is not surprising), Optional/Maybe (if all we do in the local context of the `auto var` is check for validity and then pass the variable to something else - a function, a container etc - that is not surprising).

Isn't there something more 'core' about those than 'has a foo() method returning int'? Don't they occur very often?

Thanks,

Stephen.



_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: Modernizing our use of auto

Chris Lattner via llvm-dev
In reply to this post by Chris Lattner via llvm-dev
On 31/12/2018 04:54, Chris Lattner via llvm-dev wrote:
>> 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.

One of the things which has no consensus here is whether 'auto' may be
used in lambdas (using c++-14). This feature was celebrated as a big
feature which gets unlocked by migrating to toolchains which provide
that feature:

  https://groups.google.com/forum/#!msg/llvm-dev/0VkIuhn10nE/QZ5FwYEmHAAJ

So, does this need a guideline update?

Is there consistency in celbrating that but writing 'remove all use of
auto from this file' in reviews?

If there's no consensus and no consistency, what does that mean for the
code?

Is

   if (const auto *TSI = D->getTypeSourceInfo())

ok?

Some reviewers say 'no'. What is the consensus and how is that expressed
in the guidelines?

Does anyone have any interest in making the guidelines more clear on
this?

I have made several proposals, and at least Chris agreed that something
should be improved, but then he left the discussion.

Does anyone else think that something can be improved? Is anyone willing
to read and comment on my proposal and get a change to the guidelines
committed?

The original email in this thread was about how to handle features that
become 'unlocked' by updates to our minimum toolchain requirements. That
is now upon us.

Thanks,

Stephen.

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
12