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: > > 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. 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