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>();
----------------
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.
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
