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