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

[Eduardo Caldas via Phabricator via cfe-commits]

eduucaldas added a comment.

A more general feedback.
>From our conversation one of the issues was that the tests wre only surfacing overriden methods. For instance, whenever we recorded a WalkUpFromExpr, and thus the callback showed up in the test, we actually did **not** walk up.
Now, in all the test cases we are calling the default implementation. We are not surfacing that WalkUpFrom **can** not walk up.



================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:76
+  template <typename CallDefault>
+  void recordDefaultImplementation(StringRef CallbackName,
+                                   CallDefault CallDefaultFn) {
----------------
We don't use CallbackName.


================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:107-110
+      recordCallback(__func__, IL);
+      recordDefaultImplementation(__func__, [&, this]() {
+        RecordingVisitorBase::TraverseIntegerLiteral(IL);
+      });
----------------
I think you meant to unify `recordCallback` and `recodDefaultImplentation` into one, and that's why you had added this `__func__` to `recordDefaultImplementation`.


================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:117
+      recordDefaultImplementation(
+          __func__, [&, this]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+      return true;
----------------
Do we need this `this`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82485