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

[Quentin Colombet via llvm-dev]

Le mer. 28 nov. 2018 à 09:26, Chris Lattner via llvm-dev
<llvm-dev at lists.llvm.org> a écrit :
>
> On Nov 25, 2018, at 6:43 AM, Stephen Kelly via llvm-dev <llvm-dev at lists.llvm.org> 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
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev