[PATCH] D54405: Record whether a AST Matcher constructs a Node
Zachary Turner via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 12 15:49:08 PST 2018
zturner 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>();
----------------
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?
Repository:
rC Clang
https://reviews.llvm.org/D54405
More information about the cfe-commits
mailing list