It seems like we can't match co_* keywords introduced from C++20 directly. So I try to enable the support for that.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 | ||
---|---|---|
2404 | dependend ->dependent |
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 | ||
---|---|---|
197–199 | 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. #include "my_header.h" void check_match_co_return() { co_return 1; } And test that code against coreturnStmt(isExpansionInMainFile()). |
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. |
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 | ||
---|---|---|
2393–2398 | nit: | |
2401–2402 | nit: | |
2405–2410 | nit: |
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.
LGTM with the doc nits from njames93 (and the doc generator script re-run)
+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.
nit: