I don’t really have much more to add here except to refer you to the style guide:<br><br><a href="https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable">https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable</a><br><br>Specifically this line: “Use auto if and only if it makes the code more readable or easier to maintain.”<br><br>Given that 2 out of 2 reviewers who have looked at this have said it did not make the code more readable or easier to maintain for them , and have further said that they feel the return type is not “obvious from the context”, i do not believe this usage fits within our style guidelines. <br><br>I don’t think it’s worth beating on this anymore though, because this is a lot of back and forth over something that doesn’t actually merit this much discussion. So in the interest of conforming to the style guide above, please remove auto and then start a thread on llvm-dev if you disagree with the policy <br><div class="gmail_quote"><div dir="ltr">On Tue, Nov 13, 2018 at 1:57 AM Stephen Kelly via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">steveire added inline comments.<br>
<br>
<br>
================<br>
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:77<br>
+ internal::MatcherDescriptor *matchDescriptor, StringRef MatcherName) {<br>
+ auto K = ast_type_traits::ASTNodeKind::getFromNodeKind<<br>
+ typename ast_matchers::internal::VariadicAllOfMatcher<ResultT>::Type>();<br>
----------------<br>
zturner wrote:<br>
> steveire wrote:<br>
> > aaron.ballman wrote:<br>
> > > zturner wrote:<br>
> > > > steveire wrote:<br>
> > > > > aaron.ballman wrote:<br>
> > > > > > steveire wrote:<br>
> > > > > > > aaron.ballman wrote:<br>
> > > > > > > > Mildly not keen on the use of `auto` here. It's a factory function, so it kind of names the resulting type, but it also looks like the type will be `ast_matchers::internal::VariadicAllOfMatcher<ResultT>::Type` from the template argument, which is incorrect.<br>
> > > > > > > There is no reason to assume that taking a template argument means that type is result.<br>
> > > > > > > <br>
> > > > > > > The method is `getFrom` which decreases the ambiguity still further.<br>
> > > > > > > <br>
> > > > > > > Spelling out the type doesn't add anything useful. This should be ok.<br>
> > > > > > > There is no reason to assume that taking a template argument means that type is result.<br>
> > > > > > <br>
> > > > > > Aside from all the other places that do exactly that (getAs<>, cast<>, dyn_cast<>, castAs<>, and so on). Generally, when we have a function named get that takes a template type argument, the result when seen in proximity to auto is the template type.<br>
> > > > > > <br>
> > > > > > > Spelling out the type doesn't add anything useful. This should be ok.<br>
> > > > > > <br>
> > > > > > I slept on this one and fall on the other side of it; using auto hides information that tripped up at least one person when reading the code, so don't use auto. It's not clear enough what the resulting type will be.<br>
> > > > > I put this in the category of requiring <br>
> > > > > <br>
> > > > > SomeType ST = SomeType::create();<br>
> > > > > <br>
> > > > > instead of <br>
> > > > > <br>
> > > > > auto ST = SomeType::create();<br>
> > > > > <br>
> > > > > `ast_type_traits::ASTNodeKind` is already on that line and you want to see it again.<br>
> > > > > <br>
> > > > FWIW I'm with Aaron here. Im' not familiar at all with this codebase, and looking at the code, my first instinct is "the result type is probably `ast_matchers::internal::VariadicAllOfMatcher<ResultT>::Type`". According to Aaron's earlier comment, that is incorrect, so there's at least 1 data point that it hinders readability.<br>
> > > > <br>
> > > > So, honest question. What *is* the return type here?<br>
> > > > So, honest question. What *is* the return type here?<br>
> > > <br>
> > > Much to my surprise, it's `ASTNodeKind`. It was entirely unobvious to me that this was a factory function.<br>
> > @zturner Quoting myself:<br>
> > <br>
> > > `ast_type_traits::ASTNodeKind` is already on that line and you want to see it again.<br>
> > <br>
> > The expression is `getFromNodeKind`. There is a pattern of `SomeType::fromFooKind<FooKind>()` returning a `SomeType`. This is not so unusual.<br>
> > <br>
> > <br>
> Note that at the top of this file there's already a `using namespace clang::ast_type_traits;` So if you're worried about verbosity, then you can remove the `ast_type_traits::`, remove the `auto`, and the net effect is that the overall line will end up being shorter.<br>
The funny thing is - if you see a line like <br>
<br>
Parser CodeParser = Parser::getFromArgs(Args);<br>
<br>
you have no idea what type `Parser` is! <br>
<br>
To start, it could be `clang::Parser` or `clang::ast_matchers::dynamic::Parser`, depending on a `using namespace` which might appear any distance up in the file. It is not uncommon for clang files to be thousands of lines lone.<br>
<br>
Or it could be inside a template with `template<typename Parser>`, or there could be a `using Parser = Foo;` any distance up in the file. <br>
<br>
Parser CodeParser = Parser::getFromArgs(Args);<br>
<br>
is no more helpful than<br>
<br>
auto CodeParser = Parser::getFromArgs(Args);<br>
<br>
Sticking with the same example, if `CodeParser` is a field, then you might have a line<br>
<br>
CodeParser = Parser::getFromArgs(Args);<br>
<br>
and you could object and create a macro which expands to nothing to ensure that the type appears on the line<br>
<br>
CodeParser = CALL_RETURNS(Parser)Parser::getFromArgs(Args);<br>
<br>
No one does that, because it is ok for the type to not appear on the line.<br>
<br>
You would also have to object to code such as <br>
<br>
Object.setParser(Parser::getFromArgs(Args));<br>
<br>
requiring it to instead be<br>
<br>
Parser CodeParser = Parser::getFromArgs(Args);<br>
Object.setParser(CodeParser);<br>
<br>
so that you can read the type.<br>
<br>
Even then, what if those two lines are separated by a full page of code? How do you know the type of `CodeParser` in the `Object.setParser(CodeParser)` call? The answer is you don't and you don't need to.<br>
<br>
You would also require <br>
<br>
return Parser::getFromArgs(Args);<br>
<br>
to instead be<br>
<br>
Parser CodeParser = Parser::getFromArgs(Args);<br>
return CodeParser;<br>
<br>
Claims that human readers need to see a type are as incorrect as they are inconsistent.<br>
<br>
<br>
Repository:<br>
rC Clang<br>
<br>
<a href="https://reviews.llvm.org/D54405" rel="noreferrer" target="_blank">https://reviews.llvm.org/D54405</a><br>
<br>
<br>
<br>
</blockquote></div>