This is an archive of the discontinued LLVM Phabricator instance.

Add new AST matcher `hasAnyCapture`
ClosedPublic

Authored by rhiro on Jan 8 2020, 1:45 PM.

Details

Summary

Implement new AST matcher hasAnyCapture to match on LambdaExpr captures.

Accepts child matchers cxxThisExpr to match on capture of this and also on varDecl. This is achieved by using the overload macros.

Event Timeline

rhiro created this revision.Jan 8 2020, 1:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2020, 1:45 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rhiro retitled this revision from Implement new AST matchers `hasAnyCapture` and `capturesThis`. These matchers match on LambdaExpr: hasAnyCapture - matches if any child VarDecl matcher matches one of the captured vars. capturesThis - matches if `this` is captured by the LambdaExpr. to Add new AST matchers `hasAnyCapture` and `capturesThis`.Jan 8 2020, 1:50 PM
rhiro edited the summary of this revision. (Show Details)
rhiro added a reviewer: klimek.

Thank you for this patch! You should also register the matcher(s) in Registry.cpp so that they'll work from clang-query. Also, you need to regenerate the documentation by running clang\docs\tools\dump_ast_matchers.py

clang/include/clang/ASTMatchers/ASTMatchers.h
4056

I'm a bit less certain about the utility of this matcher. I would have assumed you could do hasAnyCapture(cxxThisExpr()) to get the same behavior, but I am guessing that won't work -- though you may be able to add an overload to hasAnyCapture() to make that work. I think that might be a better approach, if it works.

rhiro updated this revision to Diff 237195.Jan 9 2020, 2:05 PM
  • Change the hasCaptureThis functionality to be an override of hasAnyCapture with cxxThisExpr argument. Also generate the documentation and update tests.
rhiro edited the summary of this revision. (Show Details)Jan 9 2020, 2:08 PM
rhiro marked an inline comment as done.Jan 9 2020, 2:29 PM

Thanks for the review! I regenerated the docs and switched the this capture case to be an override of hasAnyCapture.

clang/include/clang/ASTMatchers/ASTMatchers.h
4056

Thanks for that idea, it does seem a bit cleaner.

It would be nice if we could write a Matcher<LambdaCapture>, so that the macro composition could be more generically nested. e.g. hasAnyCapture that just loops over the LambdaCaptures and passes them to the child matcher. However, when I tried this, it became apparent that it is not a Node type that can be matched on, and would require nontrivial changes.

rhiro retitled this revision from Add new AST matchers `hasAnyCapture` and `capturesThis` to Add new AST matcher `hasAnyCapture`.Jan 9 2020, 3:34 PM
aaron.ballman added inline comments.Jan 10 2020, 7:41 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
4032–4034

I would prefer to see this written as: if (Capture.capturesVariable()) { ... } so it's a bit more resilient to other non-var captures like VLA captures.

4056

Yeah, I think this would require some work within the guts of the AST matchers and I'm not certain it would be worth the effort yet.

4058–4063

return llvm::any_of(Node.captures(), [](const LambdaCapture &LC) { return LC.capturesThis(); });

rhiro updated this revision to Diff 237389.Jan 10 2020, 10:32 AM

Fixed the condition for the varDecl case and made the cxxThisExpr more concise.

  • Condition on capturesVariable instead of capturesThis and switch the loop in the cxxThisExpr overload to use llvm::any_of instead of an explicit loop.
rhiro marked 2 inline comments as done.Jan 10 2020, 10:36 AM

I addressed the inline comments.

aaron.ballman accepted this revision.Jan 10 2020, 12:02 PM

LGTM! Do you need someone to commit on your behalf?

This revision is now accepted and ready to land.Jan 10 2020, 12:02 PM

LGTM! Do you need someone to commit on your behalf?

Yes, please.

Thank you! I have commit on your behalf in 4ffcec40acebae7161ac7426edc68290bbaca2b8