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

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

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

Alberto Barbaro 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

Alberto Barbaro 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

Alberto Barbaro 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

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

Alberto Barbaro 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

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

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

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

Alberto Barbaro 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

Alberto Barbaro 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

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

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

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