[PATCH] D82486: RecursiveASTVisitor: don't call WalkUp unnecessarily in post-order traversal

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 marked 2 inline comments as done.
gribozavr2 added inline comments.


================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:335
+  template <bool has_same_type> struct is_same_method_impl {
+    static bool isSameMethod(...) { return false; }
+  };
----------------
ymandel wrote:
> Why use var-args rather than spelling out the type arguments like you have on lines 339-341 or, simpler, line 351?
Because all of those things are not relevant. However I do see your point, that in some sense it is a trick to reduce the number of characters, but somewhat hurting readability, so I changed the signature to be explicit.


================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:339
+  template <> struct is_same_method_impl<true> {
+    template <typename FirstTy, typename FirstResult, typename... FirstParams,
+              typename SecondTy, typename SecondResult,
----------------
ymandel wrote:
> Given that we've established them to be the same type, why two sets of template arguments?
Indeed, simplified the code now!


================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:637
   case Stmt::CLASS##Class:                                                     \
-    TRY_TO(WalkUpFrom##CLASS(static_cast<CLASS *>(S))); break;
+    if (isSameMethod(&RecursiveASTVisitor::Traverse##CLASS,                    \
+                     &Derived::Traverse##CLASS)) {                             \
----------------
ymandel wrote:
> Do you explain this logic somewhere? Or do you feel it will be obvious to the reader? (I don't have a good intuition about this class to judge).
Added a comment. The RecursiveASTVisitor is full of tricky logic so no, none of this is obvious.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82486/new/

https://reviews.llvm.org/D82486