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

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

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

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

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

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

> > 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 <cfe-dev-bounces at lists.llvm.org> On Behalf Of Chandler
Carruth via cfe-dev
Sent: Tuesday, December 4, 2018 5:02 AM
To: Chris Lattner <clattner at nondot.org>
Cc: llvm-dev at lists.llvm.org; cfe-dev at lists.llvm.org; Stephen Kelly
<steveire at gmail.com>
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 <cfe-dev at
lists.llvm.org <mailto:cfe-dev at lists.llvm.org> > wrote:

Generally no IMO, because the cases that produce optional are not obvious.

 

Just to say, +1 from me too.

 

> * Can we use auto in c++14 lambda arguments with llvm::find_if(C, [](const
auto& i) { ... }) for example?
> * We need to use auto for structured bindings, if those are used.
These both get into “yes, if the type is contextually obvious”.  I’m not sure
how to specify this though.

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

 

Strongly agree.

 

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

 

And honestly, I'm very happy erring on the side of writing out the type
name. I think people are too eager to use `auto` because it is easy to write but
it makes the types substantially harder for the reader to understand.

 
> In the case that came up in review for me, the code I submitted is
> 
> template <typename BaseT, typename DerivedT>
> void registerIfNodeMatcher(...) {
>  auto NodeKind = ASTNodeKind::getFromNodeKind<DerivedT>();
>  if (!NodeKind.isNone())
>    NodeCtors[NodeKind] = std::make_pair(MatcherName, Descriptor);
> }
> 
> but it was rejected as unreadable and I was given a link to the coding
guidelines.
I agree, this seems overly strict.

 

I mean maybe. But looking at the example, I don't have a clue what type
`NodeKind` is. I think this is borderline, and I think its fine to be somewhat
subjective based on the reviewer that is more familiar with the code and APIs in
question.

 
> I'd also like to update them so that
> 
> llvm::Optional<std::pair<std::string, MatcherCtor>>
> getNodeConstructorType(ASTNodeKind targetType) {
>  auto const &ctors = RegistryData->nodeConstructors();
>  auto it = llvm::find_if(
>      ctors, [targetType](const NodeConstructorMap::value_type &ctor) {
>        return ctor.first.isSame(targetType);
>      });
>  if (it == ctors.end())
>    return llvm::None;
>  return it->second;
> }
> 
> is acceptable. The `auto it` is already acceptable, but the `auto
const& ctors` depends on an interpretation of the guideline and was rejected
in the review I submitted.
This seems like a step too far from me.  I don’t know what nodeConstructors is
here at all, and you’ve erased all type information about what you’re dealing
with.

 

Strongly agree. The `auto it` continues to seem fine, but the `ctors` here seems
super confusing to leave off all type information.

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20181204/bea7a059/attachment.html>

> 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 <
llvm-dev at lists.llvm.org> wrote:
> On Wed, Nov 28, 2018 at 6:25 PM Chris Lattner via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> Generally no IMO, because the cases that produce optional are not
obvious.
>>
>
> Just to say, +1 from me too.
>
>
>>
>> > * Can we use auto in c++14 lambda arguments with llvm::find_if(C,
>> [](const auto& i) { ... }) for example?
>> > * We need to use auto for structured bindings, if those are used.
>>
>> These both get into “yes, if the type is contextually obvious”.  I’m
not
>> sure how to specify this though.
>>
>> Perhaps the rule came be rewritten more generally, as in “auto should
>> generally only be used when the inferred type is obvious from context,
e.g.
>> as a result of a cast like dyn_cast or as a result of constructing a
value
>> with a constructor containing a type name.”?
>>
>
> Strongly agree.
>
> I understand it may not be as precise and unambiguous as people would
> like, I still think this is the correct core rule: there needs to be *some*
> reason why the type is contextually obvious.
>
> And honestly, I'm very happy erring on the side of writing out the type
> name. I think people are too eager to use `auto` because it is easy to
> write but it makes the types substantially harder for the reader to
> understand.
>
> > In the case that came up in review for me, the code I submitted is
>> >
>> > template <typename BaseT, typename DerivedT>
>> > void registerIfNodeMatcher(...) {
>> >  auto NodeKind = ASTNodeKind::getFromNodeKind<DerivedT>();
>> >  if (!NodeKind.isNone())
>> >    NodeCtors[NodeKind] = std::make_pair(MatcherName, Descriptor);
>> > }
>> >
>> > but it was rejected as unreadable and I was given a link to the
coding
>> guidelines.
>>
>> I agree, this seems overly strict.
>>
>
> I mean maybe. But looking at the example, I don't have a clue what type
> `NodeKind` is. I think this is borderline, and I think its fine to be
> somewhat subjective based on the reviewer that is more familiar with the
> code and APIs in question.
>
>
>> > I'd also like to update them so that
>> >
>> > llvm::Optional<std::pair<std::string, MatcherCtor>>
>> > getNodeConstructorType(ASTNodeKind targetType) {
>> >  auto const &ctors = RegistryData->nodeConstructors();
>> >  auto it = llvm::find_if(
>> >      ctors, [targetType](const NodeConstructorMap::value_type
&ctor) {
>> >        return ctor.first.isSame(targetType);
>> >      });
>> >  if (it == ctors.end())
>> >    return llvm::None;
>> >  return it->second;
>> > }
>> >
>> > is acceptable. The `auto it` is already acceptable, but the `auto
>> const& ctors` depends on an interpretation of the guideline and was
>> rejected in the review I submitted.
>>
>> This seems like a step too far from me.  I don’t know what
>> nodeConstructors is here at all, and you’ve erased all type information
>> about what you’re dealing with.
>>
>
> Strongly agree. The `auto it` continues to seem fine, but the `ctors` here
> seems super confusing to leave off all type information.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20181204/8da894f7/attachment-0001.html>

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 <
llvm-dev at lists.llvm.org> 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 <cfe-dev-bounces at lists.llvm.org> *On Behalf Of
*Chandler
> Carruth via cfe-dev
> *Sent:* Tuesday, December 4, 2018 5:02 AM
> *To:* Chris Lattner <clattner at nondot.org>
> *Cc:* llvm-dev at lists.llvm.org; cfe-dev at lists.llvm.org; Stephen Kelly
<
> steveire at gmail.com>
> *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 <
> cfe-dev at lists.llvm.org> wrote:
>
> Generally no IMO, because the cases that produce optional are not obvious.
>
>
>
> Just to say, +1 from me too.
>
>
>
>
> > * Can we use auto in c++14 lambda arguments with llvm::find_if(C,
> [](const auto& i) { ... }) for example?
> > * We need to use auto for structured bindings, if those are used.
>
> These both get into “yes, if the type is contextually obvious”.  I’m not
> sure how to specify this though.
>
> Perhaps the rule came be rewritten more generally, as in “auto should
> generally only be used when the inferred type is obvious from context, e.g.
> as a result of a cast like dyn_cast or as a result of constructing a value
> with a constructor containing a type name.”?
>
>
>
> Strongly agree.
>
>
>
> I understand it may not be as precise and unambiguous as people would
> like, I still think this is the correct core rule: there needs to be *some*
> reason why the type is contextually obvious.
>
>
>
> And honestly, I'm very happy erring on the side of writing out the type
> name. I think people are too eager to use `auto` because it is easy to
> write but it makes the types substantially harder for the reader to
> understand.
>
>
>
> > In the case that came up in review for me, the code I submitted is
> >
> > template <typename BaseT, typename DerivedT>
> > void registerIfNodeMatcher(...) {
> >  auto NodeKind = ASTNodeKind::getFromNodeKind<DerivedT>();
> >  if (!NodeKind.isNone())
> >    NodeCtors[NodeKind] = std::make_pair(MatcherName, Descriptor);
> > }
> >
> > but it was rejected as unreadable and I was given a link to the coding
> guidelines.
>
> I agree, this seems overly strict.
>
>
>
> I mean maybe. But looking at the example, I don't have a clue what type
> `NodeKind` is. I think this is borderline, and I think its fine to be
> somewhat subjective based on the reviewer that is more familiar with the
> code and APIs in question.
>
>
>
> > I'd also like to update them so that
> >
> > llvm::Optional<std::pair<std::string, MatcherCtor>>
> > getNodeConstructorType(ASTNodeKind targetType) {
> >  auto const &ctors = RegistryData->nodeConstructors();
> >  auto it = llvm::find_if(
> >      ctors, [targetType](const NodeConstructorMap::value_type
&ctor) {
> >        return ctor.first.isSame(targetType);
> >      });
> >  if (it == ctors.end())
> >    return llvm::None;
> >  return it->second;
> > }
> >
> > is acceptable. The `auto it` is already acceptable, but the `auto
const&
> ctors` depends on an interpretation of the guideline and was rejected in
> the review I submitted.
>
> This seems like a step too far from me.  I don’t know what
> nodeConstructors is here at all, and you’ve erased all type information
> about what you’re dealing with.
>
>
>
> Strongly agree. The `auto it` continues to seem fine, but the `ctors` here
> seems super confusing to leave off all type information.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20181204/1b937ccd/attachment.html>

> On Dec 4, 2018, at 10:59 AM, George Burgess IV <george.burgess.iv at
gmail.com> 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