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


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
  • [PATC... Stephen Kelly via Phabricator via cfe-commits
    • ... Aaron Ballman via Phabricator via cfe-commits
    • ... Stephen Kelly via Phabricator via cfe-commits
    • ... Stephen Kelly via Phabricator via cfe-commits
    • ... Aaron Ballman via Phabricator via cfe-commits
    • ... Stephen Kelly via Phabricator via cfe-commits
    • ... Zachary Turner via Phabricator via cfe-commits
    • ... Aaron Ballman via Phabricator via cfe-commits
    • ... Stephen Kelly via Phabricator via cfe-commits
    • ... Zachary Turner via Phabricator via cfe-commits
    • ... Stephen Kelly via Phabricator via cfe-commits
    • ... Stephen Kelly via Phabricator via cfe-commits
      • ... Zachary Turner via cfe-commits
        • ... Mailing List "cfe-commits" via Phabricator via cfe-commits
    • ... Stephen Kelly via Phabricator via cfe-commits
    • ... Stephen Kelly via Phabricator via cfe-commits
    • ... Aaron Ballman via Phabricator via cfe-commits

Reply via email to