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

[Chandler Carruth via llvm-dev]

On Wed, Nov 28, 2018 at 6:25 PM Chris Lattner via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

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

Just to say, +1 from me too.


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

Strongly agree.

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

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

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

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


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

Strongly agree. The `auto it` continues to seem fine, but the `ctors` here
seems super confusing to leave off all type information.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20181204/19f8e616/attachment.html>