sammccall added a comment.
This looks really sensible to me too. Some naming bikeshedding, but my main
question is migration.
Supporting two things is indeed annoying and confusing. I agree it's not worth
keeping the old way forever just to avoid writing `refersToVarDecl`.
The question is: how soon can we get rid of it? We should consider whether we
can do it in this patch: just replace the old matcher with this one.
AFAICT the old matcher has no uses in-tree beyond tests/registration. FWIW it
has no uses in our out-of-tree repo either (which generally is a heavy matchers
user).
I'm sure it has users somewhere, but probably few, and the mechanical
difficulty of updating them is low. Given that I think we should just break
them, rather than deal with overloading and deprecation warnings and future
cleanup just to put off breaking them for one more release cycle.
This is a tradeoff, to me it seems but reasonable people can disagree on the
importance of stability. Aaron, happy to go with whatever you decide.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4632
+/// refersToVarDecl(hasName("x")) matches `int x` and `x = 1`.
+AST_MATCHER_P(LambdaCapture, refersToVarDecl, internal::Matcher<VarDecl>,
+ InnerMatcher) {
----------------
I think `capturesVar` may be a clearer/more direct name.
For example given `int y; [x(y)] { return x; }` I would naively expect
`refersToVarDecl(hasName("y"))` to match the capture.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4652
+/// matches `[this]() { return cc; }`.
+AST_MATCHER(LambdaCapture, refersToThis) { return Node.capturesThis(); }
+
----------------
Again, why `refersToThis` rather than `capturesThis`, which is more specific
and matches the AST?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112491/new/
https://reviews.llvm.org/D112491
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
