<div dir="ltr"><div dir="ltr"><div dir="ltr">> 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 dir="ltr"><br></div><div>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/?</div><div><br></div><div>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:</div><div><br></div><div>> 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><div>happy.</div><div><br></div><div>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 ;)</div></div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Dec 4, 2018 at 2:02 AM Chandler Carruth via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-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"><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" target="_blank">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>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>