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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
