gribozavr updated this revision to Diff 274032.
gribozavr added a comment.
Added a FIXME about a regression.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82486/new/
https://reviews.llvm.org/D82486
Files:
clang/include/clang/AST/RecursiveASTVisitor.h
clang/lib/Tooling/Syntax/BuildTree.cpp
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
Index: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
===================================================================
--- clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
+++ clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
@@ -149,22 +149,17 @@
R"txt(
TraverseIntegerLiteral IntegerLiteral(1)
WalkUpFromStmt IntegerLiteral(1)
-WalkUpFromStmt IntegerLiteral(1)
TraverseIntegerLiteral IntegerLiteral(2)
WalkUpFromStmt IntegerLiteral(2)
-WalkUpFromStmt IntegerLiteral(2)
TraverseIntegerLiteral IntegerLiteral(3)
WalkUpFromStmt IntegerLiteral(3)
-WalkUpFromStmt IntegerLiteral(3)
WalkUpFromStmt BinaryOperator(+)
WalkUpFromStmt DeclRefExpr(add)
WalkUpFromStmt ImplicitCastExpr
TraverseIntegerLiteral IntegerLiteral(4)
WalkUpFromStmt IntegerLiteral(4)
-WalkUpFromStmt IntegerLiteral(4)
TraverseIntegerLiteral IntegerLiteral(5)
WalkUpFromStmt IntegerLiteral(5)
-WalkUpFromStmt IntegerLiteral(5)
WalkUpFromStmt CallExpr(add)
WalkUpFromStmt CompoundStmt
)txt"));
@@ -253,23 +248,14 @@
WalkUpFromIntegerLiteral IntegerLiteral(1)
WalkUpFromExpr IntegerLiteral(1)
WalkUpFromStmt IntegerLiteral(1)
-WalkUpFromIntegerLiteral IntegerLiteral(1)
- WalkUpFromExpr IntegerLiteral(1)
- WalkUpFromStmt IntegerLiteral(1)
TraverseIntegerLiteral IntegerLiteral(2)
WalkUpFromIntegerLiteral IntegerLiteral(2)
WalkUpFromExpr IntegerLiteral(2)
WalkUpFromStmt IntegerLiteral(2)
-WalkUpFromIntegerLiteral IntegerLiteral(2)
- WalkUpFromExpr IntegerLiteral(2)
- WalkUpFromStmt IntegerLiteral(2)
TraverseIntegerLiteral IntegerLiteral(3)
WalkUpFromIntegerLiteral IntegerLiteral(3)
WalkUpFromExpr IntegerLiteral(3)
WalkUpFromStmt IntegerLiteral(3)
-WalkUpFromIntegerLiteral IntegerLiteral(3)
- WalkUpFromExpr IntegerLiteral(3)
- WalkUpFromStmt IntegerLiteral(3)
WalkUpFromExpr BinaryOperator(+)
WalkUpFromStmt BinaryOperator(+)
WalkUpFromExpr DeclRefExpr(add)
@@ -280,16 +266,10 @@
WalkUpFromIntegerLiteral IntegerLiteral(4)
WalkUpFromExpr IntegerLiteral(4)
WalkUpFromStmt IntegerLiteral(4)
-WalkUpFromIntegerLiteral IntegerLiteral(4)
- WalkUpFromExpr IntegerLiteral(4)
- WalkUpFromStmt IntegerLiteral(4)
TraverseIntegerLiteral IntegerLiteral(5)
WalkUpFromIntegerLiteral IntegerLiteral(5)
WalkUpFromExpr IntegerLiteral(5)
WalkUpFromStmt IntegerLiteral(5)
-WalkUpFromIntegerLiteral IntegerLiteral(5)
- WalkUpFromExpr IntegerLiteral(5)
- WalkUpFromStmt IntegerLiteral(5)
WalkUpFromExpr CallExpr(add)
WalkUpFromStmt CallExpr(add)
WalkUpFromStmt CompoundStmt
@@ -434,13 +414,14 @@
WalkUpFromStmt IntegerLiteral(5)
)txt"));
+ // FIXME: The following log should include a call to WalkUpFromStmt for
+ // BinaryOperator(+).
EXPECT_TRUE(visitorCallbackLogEqual(
RecordingVisitor(ShouldTraversePostOrder::Yes), Code,
R"txt(
WalkUpFromStmt IntegerLiteral(1)
WalkUpFromStmt IntegerLiteral(2)
WalkUpFromStmt IntegerLiteral(3)
-WalkUpFromStmt BinaryOperator(+)
WalkUpFromStmt DeclRefExpr(add)
WalkUpFromStmt ImplicitCastExpr
WalkUpFromStmt IntegerLiteral(4)
@@ -521,6 +502,8 @@
WalkUpFromStmt IntegerLiteral(5)
)txt"));
+ // FIXME: The following log should include a call to WalkUpFromStmt for
+ // BinaryOperator(+).
EXPECT_TRUE(visitorCallbackLogEqual(
RecordingVisitor(ShouldTraversePostOrder::Yes), Code,
R"txt(
@@ -530,9 +513,6 @@
WalkUpFromStmt IntegerLiteral(2)
WalkUpFromExpr IntegerLiteral(3)
WalkUpFromStmt IntegerLiteral(3)
-WalkUpFromBinaryOperator BinaryOperator(+)
- WalkUpFromExpr BinaryOperator(+)
- WalkUpFromStmt BinaryOperator(+)
WalkUpFromExpr DeclRefExpr(add)
WalkUpFromStmt DeclRefExpr(add)
WalkUpFromExpr ImplicitCastExpr
@@ -690,7 +670,6 @@
WalkUpFromStmt IntegerLiteral(4)
WalkUpFromStmt IntegerLiteral(5)
WalkUpFromStmt CallExpr(add)
-WalkUpFromStmt CallExpr(add)
WalkUpFromStmt CompoundStmt
)txt"));
}
@@ -784,9 +763,6 @@
WalkUpFromCallExpr CallExpr(add)
WalkUpFromExpr CallExpr(add)
WalkUpFromStmt CallExpr(add)
-WalkUpFromCallExpr CallExpr(add)
- WalkUpFromExpr CallExpr(add)
- WalkUpFromStmt CallExpr(add)
WalkUpFromStmt CompoundStmt
)txt"));
}
Index: clang/lib/Tooling/Syntax/BuildTree.cpp
===================================================================
--- clang/lib/Tooling/Syntax/BuildTree.cpp
+++ clang/lib/Tooling/Syntax/BuildTree.cpp
@@ -578,15 +578,19 @@
// RAV traverses it as a statement, we produce invalid node kinds in that
// case.
// FIXME: should do this in RAV instead?
- if (S->getInit() && !TraverseStmt(S->getInit()))
- return false;
- if (S->getLoopVariable() && !TraverseDecl(S->getLoopVariable()))
- return false;
- if (S->getRangeInit() && !TraverseStmt(S->getRangeInit()))
- return false;
- if (S->getBody() && !TraverseStmt(S->getBody()))
- return false;
- return true;
+ bool Result = [&, this]() {
+ if (S->getInit() && !TraverseStmt(S->getInit()))
+ return false;
+ if (S->getLoopVariable() && !TraverseDecl(S->getLoopVariable()))
+ return false;
+ if (S->getRangeInit() && !TraverseStmt(S->getRangeInit()))
+ return false;
+ if (S->getBody() && !TraverseStmt(S->getBody()))
+ return false;
+ return true;
+ }();
+ WalkUpFromCXXForRangeStmt(S);
+ return Result;
}
bool TraverseStmt(Stmt *S) {
Index: clang/include/clang/AST/RecursiveASTVisitor.h
===================================================================
--- clang/include/clang/AST/RecursiveASTVisitor.h
+++ clang/include/clang/AST/RecursiveASTVisitor.h
@@ -331,6 +331,32 @@
struct has_same_member_pointer_type<R (T::*)(P...), R (U::*)(P...)>
: std::true_type {};
+ template <bool has_same_type> struct is_same_method_impl {
+ template <typename FirstMethodPtrTy, typename SecondMethodPtrTy>
+ static bool isSameMethod(FirstMethodPtrTy FirstMethodPtr,
+ SecondMethodPtrTy SecondMethodPtr) {
+ return false;
+ }
+ };
+
+ template <> struct is_same_method_impl<true> {
+ template <typename FirstMethodPtrTy, typename SecondMethodPtrTy>
+ static bool isSameMethod(FirstMethodPtrTy FirstMethodPtr,
+ SecondMethodPtrTy SecondMethodPtr) {
+ return FirstMethodPtr == SecondMethodPtr;
+ }
+ };
+
+ /// Returns true if and only if \p FirstMethodPtr and \p SecondMethodPtr
+ /// are pointers to the same non-static member function.
+ template <typename FirstMethodPtrTy, typename SecondMethodPtrTy>
+ bool isSameMethod(FirstMethodPtrTy FirstMethodPtr,
+ SecondMethodPtrTy SecondMethodPtr) {
+ return is_same_method_impl<
+ has_same_member_pointer_type<FirstMethodPtrTy, SecondMethodPtrTy>::
+ value>::isSameMethod(FirstMethodPtr, SecondMethodPtr);
+ }
+
// Traverse the given statement. If the most-derived traverse function takes a
// data recursion queue, pass it on; otherwise, discard it. Note that the
// first branch of this conditional must compile whether or not the derived
@@ -609,17 +635,38 @@
#define ABSTRACT_STMT(STMT)
#define STMT(CLASS, PARENT) \
case Stmt::CLASS##Class: \
- TRY_TO(WalkUpFrom##CLASS(static_cast<CLASS *>(S))); break;
+ /* In pre-order traversal mode, each Traverse##STMT method is responsible \
+ * for calling WalkUpFrom. Therefore, if the user overrides Traverse##STMT \
+ * and does not call the default implementation, the WalkUpFrom callback \
+ * is not called. Post-order traversal mode should provide the same \
+ * behavior regarding method overrides. \
+ * \
+ * In post-order traversal mode the Traverse##STMT method, when it \
+ * receives a DataRecursionQueue, can't call WalkUpFrom after traversing \
+ * children because it only enqueues the children and does not traverse \
+ * them. TraverseStmt traverses the enqueued children, and we call \
+ * WalkUpFrom here. \
+ * \
+ * However, to make pre-order and post-order modes identical with regards \
+ * to whether they call WalkUpFrom at all, we call WalkUpFrom if and only \
+ * if the user did not override the Traverse##STMT method. */ \
+ if (isSameMethod(&RecursiveASTVisitor::Traverse##CLASS, \
+ &Derived::Traverse##CLASS)) { \
+ TRY_TO(WalkUpFrom##CLASS(static_cast<CLASS *>(S))); \
+ } \
+ break;
#define INITLISTEXPR(CLASS, PARENT) \
case Stmt::CLASS##Class: \
- { \
+ /* See the comment above for the explanation of the isSameMethod check. */ \
+ if (isSameMethod(&RecursiveASTVisitor::Traverse##CLASS, \
+ &Derived::Traverse##CLASS)) { \
auto ILE = static_cast<CLASS *>(S); \
if (auto Syn = ILE->isSemanticForm() ? ILE->getSyntacticForm() : ILE) \
TRY_TO(WalkUpFrom##CLASS(Syn)); \
if (auto Sem = ILE->isSemanticForm() ? ILE : ILE->getSemanticForm()) \
TRY_TO(WalkUpFrom##CLASS(Sem)); \
- break; \
- }
+ } \
+ break;
#include "clang/AST/StmtNodes.inc"
}
@@ -2220,8 +2267,13 @@
TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(SubStmt); \
} \
} \
- if (!Queue && ReturnValue && getDerived().shouldTraversePostOrder()) \
+ /* Call WalkUpFrom if TRY_TO_TRAVERSE_OR_ENQUEUE_STMT has traversed the \
+ * children already. If TRY_TO_TRAVERSE_OR_ENQUEUE_STMT only enqueued the \
+ * children, PostVisitStmt will call WalkUpFrom after we are done visiting \
+ * children. */ \
+ if (!Queue && ReturnValue && getDerived().shouldTraversePostOrder()) { \
TRY_TO(WalkUpFrom##STMT(S)); \
+ } \
return ReturnValue; \
}
@@ -2392,6 +2444,9 @@
for (Stmt *SubStmt : S->children()) {
TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(SubStmt);
}
+
+ if (!Queue && getDerived().shouldTraversePostOrder())
+ TRY_TO(WalkUpFromInitListExpr(S));
}
return true;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
