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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 marked an inline comment as done.
gribozavr2 added inline comments.


================
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:117
+      recordDefaultImplementation(
+          __func__, [&, this]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+      return true;
----------------
eduucaldas wrote:
> gribozavr2 wrote:
> > 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);`.
> True! The `&` default captures `this` already, but a better suggestion would be to use the capture: `[S, this]` or perhaps to just use `S` as an argument of the lambda. 
> That is a nitpick though, feel free to push the patch :)
> The & default captures this already

Oh indeed, thanks! I changed the capture list to `[&]` because I think it is not interesting what this closure captures exactly. It is not stored anywhere beyond the `recordCallback` call and invoked only once, so there are no lifetime concerns for variables captured by reference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82485