[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

> Now, in all the test cases we are calling the default implementation. We are not surfacing that WalkUpFrom can not walk up.

I think we are. The log of callbacks called from the default implementation is indented to the right, so we clearly see what the default implementation does, and what the behavior would have been if we didn't call the default implementation.



================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:76
+  template <typename CallDefault>
+  void recordDefaultImplementation(StringRef CallbackName,
+                                   CallDefault CallDefaultFn) {
----------------
eduucaldas wrote:
> We don't use CallbackName.
Removed (initially I had a different implementation).


================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:107-110
+      recordCallback(__func__, IL);
+      recordDefaultImplementation(__func__, [&, this]() {
+        RecordingVisitorBase::TraverseIntegerLiteral(IL);
+      });
----------------
eduucaldas wrote:
> I think you meant to unify `recordCallback` and `recodDefaultImplentation` into one, and that's why you had added this `__func__` to `recordDefaultImplementation`.
I was passing `__func__` to `recordDefaultImplementation` for a different reason (I had a different output format initially), but unifying the two functions like you're suggesting makes a lot of sense. Changed the code to do that.


================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:117
+      recordDefaultImplementation(
+          __func__, [&, this]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+      return true;
----------------
eduucaldas wrote:
> Do we need this `this`?
Yes, we're calling a method on `this` from the base class. `RecordingVisitorBase::WalkUpFromStmt(S);` is implicitly calling a member function, `this-> RecordingVisitorBase::WalkUpFromStmt(S);`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82485