[PATCH] D54405: Record whether a AST Matcher constructs a Node

[Zachary Turner via cfe-commits]

I don’t really have much more to add here except to refer you to the style
guide:

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

Specifically this line: “Use auto if and only if it makes the code more
readable or easier to maintain.”

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.

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
On Tue, Nov 13, 2018 at 1:57 AM Stephen Kelly via Phabricator <
reviews at reviews.llvm.org> wrote:

> steveire added inline comments.
>
>
> ================
> Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:77
> +      internal::MatcherDescriptor *matchDescriptor, StringRef
> MatcherName) {
> +    auto K = ast_type_traits::ASTNodeKind::getFromNodeKind<
> +        typename
> ast_matchers::internal::VariadicAllOfMatcher<ResultT>::Type>();
> ----------------
> zturner wrote:
> > steveire wrote:
> > > aaron.ballman wrote:
> > > > zturner wrote:
> > > > > steveire wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > steveire wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > 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.
> > > > > > > > There is no reason to assume that taking a template argument
> means that type is result.
> > > > > > > >
> > > > > > > > The method is `getFrom` which decreases the ambiguity still
> further.
> > > > > > > >
> > > > > > > > Spelling out the type doesn't add anything useful. This
> should be ok.
> > > > > > > > There is no reason to assume that taking a template argument
> means that type is result.
> > > > > > >
> > > > > > > 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.
> > > > > > >
> > > > > > > > Spelling out the type doesn't add anything useful. This
> should be ok.
> > > > > > >
> > > > > > > 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.
> > > > > > I put this in the category of requiring
> > > > > >
> > > > > >     SomeType ST = SomeType::create();
> > > > > >
> > > > > > instead of
> > > > > >
> > > > > >     auto ST = SomeType::create();
> > > > > >
> > > > > > `ast_type_traits::ASTNodeKind` is already on that line and you
> want to see it again.
> > > > > >
> > > > > 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.
> > > > >
> > > > > So, honest question.  What *is* the return type here?
> > > > > So, honest question. What *is* the return type here?
> > > >
> > > > Much to my surprise, it's `ASTNodeKind`. It was entirely unobvious
> to me that this was a factory function.
> > > @zturner Quoting myself:
> > >
> > > > `ast_type_traits::ASTNodeKind` is already on that line and you want
> to see it again.
> > >
> > > The expression is `getFromNodeKind`. There is a pattern of
> `SomeType::fromFooKind<FooKind>()` returning a `SomeType`. This is not so
> unusual.
> > >
> > >
> > 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.
> The funny thing is - if you see a line like
>
>     Parser CodeParser = Parser::getFromArgs(Args);
>
> you have no idea what type `Parser` is!
>
> 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.
>
> 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.
>
>     Parser CodeParser = Parser::getFromArgs(Args);
>
> is no more helpful than
>
>     auto CodeParser = Parser::getFromArgs(Args);
>
> Sticking with the same example, if `CodeParser` is a field, then you might
> have a line
>
>     CodeParser = Parser::getFromArgs(Args);
>
> and you could object and create a macro which expands to nothing to ensure
> that the type appears on the line
>
>     CodeParser = CALL_RETURNS(Parser)Parser::getFromArgs(Args);
>
> No one does that, because it is ok for the type to not appear on the line.
>
> You would also have to object to code such as
>
>     Object.setParser(Parser::getFromArgs(Args));
>
> requiring it to instead be
>
>     Parser CodeParser = Parser::getFromArgs(Args);
>     Object.setParser(CodeParser);
>
> so that you can read the type.
>
> 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.
>
> You would also require
>
>     return Parser::getFromArgs(Args);
>
> to instead be
>
>     Parser CodeParser = Parser::getFromArgs(Args);
>     return CodeParser;
>
> Claims that human readers need to see a type are as incorrect as they are
> inconsistent.
>
>
> Repository:
>   rC Clang
>
> https://reviews.llvm.org/D54405
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181113/26404aae/attachment-0001.html>