<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Wed, Nov 28, 2018 at 6:25 PM Chris Lattner via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Generally no IMO, because the cases that produce optional are not obvious.<br></blockquote><div><br></div><div>Just to say, +1 from me too.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> * Can we use auto in c++14 lambda arguments with llvm::find_if(C, [](const auto& i) { ... }) for example?<br>
> * We need to use auto for structured bindings, if those are used.<br>
<br>
These both get into “yes, if the type is contextually obvious”.  I’m not sure how to specify this though.<br>
<br>
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.”?<br></blockquote><div><br></div><div>Strongly agree.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> In the case that came up in review for me, the code I submitted is<br>
> <br>
> template <typename BaseT, typename DerivedT><br>
> void registerIfNodeMatcher(...) {<br>
>  auto NodeKind = ASTNodeKind::getFromNodeKind<DerivedT>();<br>
>  if (!NodeKind.isNone())<br>
>    NodeCtors[NodeKind] = std::make_pair(MatcherName, Descriptor);<br>
> }<br>
> <br>
> but it was rejected as unreadable and I was given a link to the coding guidelines.<br>
<br>
I agree, this seems overly strict.<br></blockquote><div><br></div><div>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.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> I'd also like to update them so that<br>
> <br>
> llvm::Optional<std::pair<std::string, MatcherCtor>><br>
> getNodeConstructorType(ASTNodeKind targetType) {<br>
>  auto const &ctors = RegistryData->nodeConstructors();<br>
>  auto it = llvm::find_if(<br>
>      ctors, [targetType](const NodeConstructorMap::value_type &ctor) {<br>
>        return ctor.first.isSame(targetType);<br>
>      });<br>
>  if (it == ctors.end())<br>
>    return llvm::None;<br>
>  return it->second;<br>
> }<br>
> <br>
> 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.<br>
<br>
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.<br></blockquote><div><br></div><div>Strongly agree. The `auto it` continues to seem fine, but the `ctors` here seems super confusing to leave off all type information.</div></div></div>