aaron.ballman added a comment.
Thanks for this -- it looks like really interesting functionality! I've mostly
found nits thus far, but did have a question about clang-query support for it.
================
Comment at: include/clang/AST/ASTContext.h:562-563
public:
+ ast_type_traits::TraversalKind GetTraversalKind() const { return Traversal; }
+ void SetTraversalKind(ast_type_traits::TraversalKind TK) { Traversal = TK; }
+
----------------
This doesn't match our coding guidelines. Should be `getTraversalKind()`, etc.
Same below.
================
Comment at: include/clang/AST/ASTContext.h:565-568
+ const Expr *TraverseIgnored(const Expr *E);
+ Expr *TraverseIgnored(Expr *E);
+ ast_type_traits::DynTypedNode
+ TraverseIgnored(const ast_type_traits::DynTypedNode &N);
----------------
Is there a reason we don't want these functions to be marked `const`?
================
Comment at: include/clang/AST/ASTNodeTraverser.h:80
+ void SetTraversalKind(ast_type_traits::TraversalKind TK) { Traversal = TK; }
+
----------------
`setTraversalKind()`
================
Comment at: include/clang/AST/ASTNodeTraverser.h:105
- void Visit(const Stmt *S, StringRef Label = {}) {
+ void Visit(const Stmt *S_, StringRef Label = {}) {
getNodeDelegate().AddChild(Label, [=] {
----------------
Let's not make underscores important like this -- how about `Node` or something
generic instead?
================
Comment at: include/clang/AST/ASTNodeTraverser.h:113-114
+ break;
+ case ast_type_traits::TK_IgnoreImplicitCastsAndParentheses:
+ S = E->IgnoreParenImpCasts();
+ break;
----------------
What an unfortunate name for this. `IgnoreParenImpCasts()` ignores parens,
implicit casts, full expressions, temporary materialization, and non-type
template substitutions, and those extra nodes have surprised people in the past.
Not much to be done about it here, just loudly lamenting the existing name of
the trait.
================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:701
+/// Causes all nested matchers to be matched with the specified traversal kind
+///
----------------
Missing a full stop at the end of the comment.
================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:716
+/// \endcode
+/// matches the return statement with "ret" bound to "a".
+template <typename T>
----------------
Copy pasta?
================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:718
+template <typename T>
+internal::Matcher<T> traverse(ast_type_traits::TraversalKind TK,
+ const internal::Matcher<T> &InnerMatcher) {
----------------
Is this an API we should be exposing to clang-query as well? Will those users
be able to use a string literal for the traversal kind, like they already do
for attribute kinds (etc)?
================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:286
+
+ virtual llvm::Optional<ast_type_traits::TraversalKind> TraversalKind() const
{
+ return {};
----------------
`traversalKind()`
================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:287
+ virtual llvm::Optional<ast_type_traits::TraversalKind> TraversalKind() const
{
+ return {};
+ }
----------------
`return llvm::None;`
================
Comment at: lib/AST/ASTContext.cpp:120
+ASTContext::TraverseIgnored(const ast_type_traits::DynTypedNode &N) {
+ if (auto E = N.get<Expr>()) {
+ return ast_type_traits::DynTypedNode::create(*TraverseIgnored(E));
----------------
`auto *`
================
Comment at: lib/AST/ASTContext.cpp:10358
bool TraverseStmt(Stmt *StmtNode) {
+ auto FilteredNode = StmtNode;
+ if (auto *ExprNode = dyn_cast_or_null<Expr>(FilteredNode))
----------------
`Stmt *`
================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:148
Stmt *StmtToTraverse = StmtNode;
+ if (Expr *ExprNode = dyn_cast_or_null<Expr>(StmtNode))
+ StmtToTraverse = Finder->getASTContext().TraverseIgnored(ExprNode);
----------------
`auto *`
================
Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:214-215
BoundNodesTreeBuilder *Builder) const {
- if (RestrictKind.isBaseOf(DynNode.getNodeKind()) &&
- Implementation->dynMatches(DynNode, Finder, Builder)) {
+ auto PreviousTraversalKind = Finder->getASTContext().GetTraversalKind();
+ auto OptTK = Implementation->TraversalKind();
+ if (OptTK)
----------------
Please do not use `auto` here.
================
Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:218-219
+ Finder->getASTContext().SetTraversalKind(*OptTK);
+ auto N = Finder->getASTContext().TraverseIgnored(DynNode);
+ auto NodeKind = N.getNodeKind();
+
----------------
Or here...
================
Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:237-238
BoundNodesTreeBuilder *Builder) const {
- assert(RestrictKind.isBaseOf(DynNode.getNodeKind()));
- if (Implementation->dynMatches(DynNode, Finder, Builder)) {
+ auto N = Finder->getASTContext().TraverseIgnored(DynNode);
+ auto NodeKind = N.getNodeKind();
+
----------------
Or here...
================
Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:240
+
+ assert(RestrictKind.isBaseOf(NodeKind));
+ if (Implementation->dynMatches(N, Finder, Builder)) {
----------------
Add an assertion message?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61837/new/
https://reviews.llvm.org/D61837
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
