This is an archive of the discontinued LLVM Phabricator instance.

[ASTMatcher] Add AST Matcher support for C++20 coroutine keywords
ClosedPublic

Authored by ChuanqiXu on Feb 8 2021, 9:51 PM.

Details

Summary

It seems like we can't match co_* keywords introduced from C++20 directly. So I try to enable the support for that.

Diff Detail

Event Timeline

ChuanqiXu requested review of this revision.Feb 8 2021, 9:51 PM
ChuanqiXu created this revision.

This looks good, as far as it goes - it seems like you'll want the ability to match the wrapped expression too, but I don't see a reason that needs to be in this patch.

You'll need to run clang/docs/tools/dump_ast_matchers.py to update the docs for the new matchers.

clang/include/clang/ASTMatchers/ASTMatchers.h
2414

dependend ->dependent

njames93 requested changes to this revision.Feb 9 2021, 3:46 PM
njames93 added a subscriber: njames93.

Small stylistic nit, is it better to have these matchers named coAwaitExpr, coYieldExpr etc. I know the types in clang aren't capitalized but they are internal to clang. Matchers are user facing so having a nicer style is more important.

clang/lib/ASTMatchers/Dynamic/Registry.cpp
195–197

Please keep this in alphabetical order.

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
559–572

This is a little unsatisfactory for a test. You aren't ensuring that coReturnStmt() is matching the co_return etc.
A better test could creating a FileContentMappings object and use it to create a header file with all the boilerplate.
Then you can have test code like this:

#include "my_header.h"
void check_match_co_return() {
  co_return 1;
}

And test that code against coreturnStmt(isExpansionInMainFile()).
This will be a very precise test case.
You'd have to use matchesConditionally instead of matches in order to pass the headers.

This revision now requires changes to proceed.Feb 9 2021, 3:46 PM
ChuanqiXu updated this revision to Diff 324490.Feb 17 2021, 6:33 PM

Fix typos and update test cases.

ChuanqiXu updated this revision to Diff 324493.Feb 17 2021, 6:40 PM

update clang/docs/LibASTMatchersReference.html

it seems like you'll want the ability to match the wrapped expression too, but I don't see a reason that needs to be in this patch.

I am confused about the statements? What does it mean?

it seems like you'll want the ability to match the wrapped expression too, but I don't see a reason that needs to be in this patch.

I am confused about the statements? What does it mean?

He's talking about Traversal Matchers. Though they could go in a follow up patch.
Basically matchers that let one write:

coyieldExpr(yeilds(...))
coreturnStmt(returns(...))
coawaitExpr(hasOperand(...))

it seems like you'll want the ability to match the wrapped expression too, but I don't see a reason that needs to be in this patch.

I am confused about the statements? What does it mean?

He's talking about Traversal Matchers. Though they could go in a follow up patch.
Basically matchers that let one write:

coyieldExpr(yeilds(...))
coreturnStmt(returns(...))
coawaitExpr(hasOperand(...))

I am not familiar with ASTMatchers. And from the link you give, it looks like that coyieldExpr, coreturnStmt and coawaitExpr should be Node Matchers instead of Traversal Matchers.

He's talking about Traversal Matchers. Though they could go in a follow up patch.
Basically matchers that let one write:

coyieldExpr(yeilds(...))
coreturnStmt(returns(...))
coawaitExpr(hasOperand(...))

I am not familiar with ASTMatchers. And from the link you give, it looks like that coyieldExpr, coreturnStmt and coawaitExpr should be Node Matchers instead of Traversal Matchers.

Those should be Node matchers, the traversal matchers would be the yields(...), returns(...) and hasOperand(...) matchers.

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
569

nit: use c++20, same for the tests below.

ChuanqiXu updated this revision to Diff 326335.Feb 25 2021, 2:54 AM

update c++20 with c++2a

He's talking about Traversal Matchers. Though they could go in a follow up patch.
Basically matchers that let one write:

coyieldExpr(yeilds(...))
coreturnStmt(returns(...))
coawaitExpr(hasOperand(...))

I am not familiar with ASTMatchers. And from the link you give, it looks like that coyieldExpr, coreturnStmt and coawaitExpr should be Node Matchers instead of Traversal Matchers.

Those should be Node matchers, the traversal matchers would be the yields(...), returns(...) and hasOperand(...) matchers.

So is it ok to omit the implementation like yields ?

So is it ok to omit the implementation like yields ?

Yes, that can be done in a follow up and there's no onus on you to do it.

So is it ok to omit the implementation like yields ?

Yes, that can be done in a follow up and there's no onus on you to do it.

Thanks for reviewing! Do you think is it ok to commit this patch?

I'm still of the opinion we should be capitalizing these, coYieldExpr etc, but I'd prefer feedback from other reviewers before making that change.
Also some small changes to the matchers documentation while will require re running the documentation script.

clang/include/clang/ASTMatchers/ASTMatchers.h
2403–2408

nit:

2411–2412

nit:

2415–2420

nit:

I'm still of the opinion we should be capitalizing these, coYieldExpr etc, but I'd prefer feedback from other reviewers before making that change.
Also some small changes to the matchers documentation while will require re running the documentation script.

The convention which is almost completely followed currently is to use the name of the AST class and lowercase the prefix (ie CXX to cxx). In the AST class, the Co is not a prefix, because the AST class is not called CoYieldExpr but CoyieldExpr.

So, I think the matchers should continue to follow the convention and be called coyieldExpr.

sammccall accepted this revision.Mar 9 2021, 1:50 PM

LGTM with the doc nits from njames93 (and the doc generator script re-run)

The convention which is almost completely followed currently is to use the name of the AST class and lowercase the prefix (ie CXX to cxx). In the AST class, the Co is not a prefix, because the AST class is not called CoYieldExpr but CoyieldExpr.

So, I think the matchers should continue to follow the convention and be called coyieldExpr.

+1, it's more important (and realistic) to match the names of the AST nodes than any other names.

+1, it's more important (and realistic) to match the names of the AST nodes than any other names.

I think my gripe is that in my opinion the ast nodes should have be named CoYieldExpr etc.

ChuanqiXu updated this revision to Diff 329923.Mar 11 2021, 4:32 AM

Re-run documentation with the nit suggested.

Thanks for reviewing!

aaron.ballman accepted this revision.Mar 11 2021, 4:49 AM

LGTM

clang/include/clang/ASTMatchers/ASTMatchers.h
2413
njames93 accepted this revision.Mar 11 2021, 12:54 PM
This revision is now accepted and ready to land.Mar 11 2021, 12:54 PM
This revision was landed with ongoing or failed builds.Mar 21 2021, 7:28 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2021, 7:28 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript