aaron.ballman added a comment.
Aside from the uses of auto and the lack of tests, this LGTM.
================
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:
> > 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.
Repository:
rC Clang
https://reviews.llvm.org/D54405
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
