[cfe-dev] [llvm-dev] RFC: Modernizing our use of auto
Chandler Carruth via cfe-dev
cfe-dev at lists.llvm.org
Tue Dec 4 02:01:37 PST 2018
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/cfe-dev/attachments/20181204/19f8e616/attachment.html>
More information about the cfe-dev
mailing list