[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces
nemanjai added a comment. Hi @gribozavr do you think we can do something about this test? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82485/new/ https://reviews.llvm.org/D82485 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces
nemanjai added a comment. Is there a way this test case can somehow be broken up into multiple files? The test case takes a very long time to compile which causes intermittent but frequent failures on one of our bots that runs on a fairly small VM. Most of the failures listed here: http://lab.llvm.org:8011/builders/clang-ppc64le-linux?numbuilds=100 are timeout failures when compiling this test. It seems unfortunate to have to increase the timeout limit on the bot just for this - especially if this can be broken up into multiple files. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82485/new/ https://reviews.llvm.org/D82485 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8e5a56865f28: Add tests for sequences of callbacks that
RecursiveASTVisitor produces (authored by gribozavr).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82485/new/
https://reviews.llvm.org/D82485
Files:
clang/unittests/Tooling/CMakeLists.txt
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
Index: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
@@ -0,0 +1,877 @@
+//===--- clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "TestVisitor.h"
+
+using namespace clang;
+
+namespace {
+
+enum class ShouldTraversePostOrder : bool {
+ No = false,
+ Yes = true,
+};
+
+/// Base class for tests for RecursiveASTVisitor tests that validate the
+/// sequence of calls to user-defined callbacks like Traverse*(), WalkUp*(),
+/// Visit*().
+template
+class RecordingVisitorBase : public TestVisitor {
+ ShouldTraversePostOrder ShouldTraversePostOrderValue;
+
+public:
+ RecordingVisitorBase(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+ : ShouldTraversePostOrderValue(ShouldTraversePostOrderValue) {}
+
+ bool shouldTraversePostOrder() const {
+return static_cast(ShouldTraversePostOrderValue);
+ }
+
+ // Callbacks received during traversal.
+ std::string CallbackLog;
+ unsigned CallbackLogIndent = 0;
+
+ std::string stmtToString(Stmt *S) {
+StringRef ClassName = S->getStmtClassName();
+if (IntegerLiteral *IL = dyn_cast(S)) {
+ return (ClassName + "(" + IL->getValue().toString(10, false) + ")").str();
+}
+if (BinaryOperator *BO = dyn_cast(S)) {
+ return (ClassName + "(" + BinaryOperator::getOpcodeStr(BO->getOpcode()) +
+ ")")
+ .str();
+}
+if (CallExpr *CE = dyn_cast(S)) {
+ if (FunctionDecl *Callee = CE->getDirectCallee()) {
+if (Callee->getIdentifier()) {
+ return (ClassName + "(" + Callee->getName() + ")").str();
+}
+ }
+}
+if (DeclRefExpr *DRE = dyn_cast(S)) {
+ if (NamedDecl *ND = DRE->getFoundDecl()) {
+if (ND->getIdentifier()) {
+ return (ClassName + "(" + ND->getName() + ")").str();
+}
+ }
+}
+return ClassName.str();
+ }
+
+ /// Record the fact that the user-defined callback member function
+ /// \p CallbackName was called with the argument \p S. Then, record the
+ /// effects of calling the default implementation \p CallDefaultFn.
+ template
+ void recordCallback(StringRef CallbackName, Stmt *S,
+ CallDefault CallDefaultFn) {
+for (unsigned i = 0; i != CallbackLogIndent; ++i) {
+ CallbackLog += " ";
+}
+CallbackLog += (CallbackName + " " + stmtToString(S) + "\n").str();
+++CallbackLogIndent;
+CallDefaultFn();
+--CallbackLogIndent;
+ }
+};
+
+template
+::testing::AssertionResult visitorCallbackLogEqual(VisitorTy Visitor,
+ StringRef Code,
+ StringRef ExpectedLog) {
+ Visitor.runOver(Code);
+ // EXPECT_EQ shows the diff between the two strings if they are different.
+ EXPECT_EQ(ExpectedLog.trim().str(),
+StringRef(Visitor.CallbackLog).trim().str());
+ if (ExpectedLog.trim() != StringRef(Visitor.CallbackLog).trim()) {
+return ::testing::AssertionFailure();
+ }
+ return ::testing::AssertionSuccess();
+}
+
+} // namespace
+
+TEST(RecursiveASTVisitor, StmtCallbacks_TraverseLeaf) {
+ class RecordingVisitor : public RecordingVisitorBase {
+ public:
+RecordingVisitor(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+: RecordingVisitorBase(ShouldTraversePostOrderValue) {}
+
+bool TraverseIntegerLiteral(IntegerLiteral *IL) {
+ recordCallback(__func__, IL, [&]() {
+RecordingVisitorBase::TraverseIntegerLiteral(IL);
+ });
+ return true;
+}
+
+bool WalkUpFromStmt(Stmt *S) {
+ recordCallback(__func__, S,
+ [&]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+ return true;
+}
+ };
+
+ StringRef Code = R"cpp(
+void add(int, int);
+void test() {
+ 1;
+ 2 + 3;
+ add(4, 5);
+}
+)cpp";
+
+ EXPECT_TRUE(visitorCallbackLogEqual(
+ RecordingVisitor(ShouldTraversePostOrder::No), Code,
+ R"txt(
+WalkUpFromStmt CompoundStmt
+TraverseIntegerLiteral IntegerLiteral(1)
+ WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromStmt BinaryOperator(+)
+TraverseIntegerLiteral
[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces
gribozavr updated this revision to Diff 274010.
gribozavr added a comment.
Simplified capture lists in lambdas
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82485/new/
https://reviews.llvm.org/D82485
Files:
clang/unittests/Tooling/CMakeLists.txt
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
Index: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
@@ -0,0 +1,877 @@
+//===--- clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "TestVisitor.h"
+
+using namespace clang;
+
+namespace {
+
+enum class ShouldTraversePostOrder : bool {
+ No = false,
+ Yes = true,
+};
+
+/// Base class for tests for RecursiveASTVisitor tests that validate the
+/// sequence of calls to user-defined callbacks like Traverse*(), WalkUp*(),
+/// Visit*().
+template
+class RecordingVisitorBase : public TestVisitor {
+ ShouldTraversePostOrder ShouldTraversePostOrderValue;
+
+public:
+ RecordingVisitorBase(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+ : ShouldTraversePostOrderValue(ShouldTraversePostOrderValue) {}
+
+ bool shouldTraversePostOrder() const {
+return static_cast(ShouldTraversePostOrderValue);
+ }
+
+ // Callbacks received during traversal.
+ std::string CallbackLog;
+ unsigned CallbackLogIndent = 0;
+
+ std::string stmtToString(Stmt *S) {
+StringRef ClassName = S->getStmtClassName();
+if (IntegerLiteral *IL = dyn_cast(S)) {
+ return (ClassName + "(" + IL->getValue().toString(10, false) + ")").str();
+}
+if (BinaryOperator *BO = dyn_cast(S)) {
+ return (ClassName + "(" + BinaryOperator::getOpcodeStr(BO->getOpcode()) +
+ ")")
+ .str();
+}
+if (CallExpr *CE = dyn_cast(S)) {
+ if (FunctionDecl *Callee = CE->getDirectCallee()) {
+if (Callee->getIdentifier()) {
+ return (ClassName + "(" + Callee->getName() + ")").str();
+}
+ }
+}
+if (DeclRefExpr *DRE = dyn_cast(S)) {
+ if (NamedDecl *ND = DRE->getFoundDecl()) {
+if (ND->getIdentifier()) {
+ return (ClassName + "(" + ND->getName() + ")").str();
+}
+ }
+}
+return ClassName.str();
+ }
+
+ /// Record the fact that the user-defined callback member function
+ /// \p CallbackName was called with the argument \p S. Then, record the
+ /// effects of calling the default implementation \p CallDefaultFn.
+ template
+ void recordCallback(StringRef CallbackName, Stmt *S,
+ CallDefault CallDefaultFn) {
+for (unsigned i = 0; i != CallbackLogIndent; ++i) {
+ CallbackLog += " ";
+}
+CallbackLog += (CallbackName + " " + stmtToString(S) + "\n").str();
+++CallbackLogIndent;
+CallDefaultFn();
+--CallbackLogIndent;
+ }
+};
+
+template
+::testing::AssertionResult visitorCallbackLogEqual(VisitorTy Visitor,
+ StringRef Code,
+ StringRef ExpectedLog) {
+ Visitor.runOver(Code);
+ // EXPECT_EQ shows the diff between the two strings if they are different.
+ EXPECT_EQ(ExpectedLog.trim().str(),
+StringRef(Visitor.CallbackLog).trim().str());
+ if (ExpectedLog.trim() != StringRef(Visitor.CallbackLog).trim()) {
+return ::testing::AssertionFailure();
+ }
+ return ::testing::AssertionSuccess();
+}
+
+} // namespace
+
+TEST(RecursiveASTVisitor, StmtCallbacks_TraverseLeaf) {
+ class RecordingVisitor : public RecordingVisitorBase {
+ public:
+RecordingVisitor(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+: RecordingVisitorBase(ShouldTraversePostOrderValue) {}
+
+bool TraverseIntegerLiteral(IntegerLiteral *IL) {
+ recordCallback(__func__, IL, [&]() {
+RecordingVisitorBase::TraverseIntegerLiteral(IL);
+ });
+ return true;
+}
+
+bool WalkUpFromStmt(Stmt *S) {
+ recordCallback(__func__, S,
+ [&]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+ return true;
+}
+ };
+
+ StringRef Code = R"cpp(
+void add(int, int);
+void test() {
+ 1;
+ 2 + 3;
+ add(4, 5);
+}
+)cpp";
+
+ EXPECT_TRUE(visitorCallbackLogEqual(
+ RecordingVisitor(ShouldTraversePostOrder::No), Code,
+ R"txt(
+WalkUpFromStmt CompoundStmt
+TraverseIntegerLiteral IntegerLiteral(1)
+ WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromStmt BinaryOperator(+)
+TraverseIntegerLiteral IntegerLiteral(2)
+ WalkUpFromStmt IntegerLiteral(2)
+TraverseIntegerLiteral
[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces
gribozavr2 marked an inline comment as done.
gribozavr2 added inline comments.
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:117
+ recordDefaultImplementation(
+ __func__, [&, this]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+ return true;
eduucaldas wrote:
> gribozavr2 wrote:
> > eduucaldas wrote:
> > > Do we need this `this`?
> > Yes, we're calling a method on `this` from the base class.
> > `RecordingVisitorBase::WalkUpFromStmt(S);` is implicitly calling a member
> > function, `this-> RecordingVisitorBase::WalkUpFromStmt(S);`.
> True! The `&` default captures `this` already, but a better suggestion would
> be to use the capture: `[S, this]` or perhaps to just use `S` as an argument
> of the lambda.
> That is a nitpick though, feel free to push the patch :)
> The & default captures this already
Oh indeed, thanks! I changed the capture list to `[&]` because I think it is
not interesting what this closure captures exactly. It is not stored anywhere
beyond the `recordCallback` call and invoked only once, so there are no
lifetime concerns for variables captured by reference.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82485/new/
https://reviews.llvm.org/D82485
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces
eduucaldas accepted this revision.
eduucaldas added inline comments.
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:117
+ recordDefaultImplementation(
+ __func__, [&, this]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+ return true;
gribozavr2 wrote:
> eduucaldas wrote:
> > Do we need this `this`?
> Yes, we're calling a method on `this` from the base class.
> `RecordingVisitorBase::WalkUpFromStmt(S);` is implicitly calling a member
> function, `this-> RecordingVisitorBase::WalkUpFromStmt(S);`.
True! The `&` default captures `this` already, but a better suggestion would be
to use the capture: `[S, this]` or perhaps to just use `S` as an argument of
the lambda.
That is a nitpick though, feel free to push the patch :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82485/new/
https://reviews.llvm.org/D82485
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces
gribozavr updated this revision to Diff 273835.
gribozavr added a comment.
Unified recordCallback and recordDefaultImplementation into one call.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82485/new/
https://reviews.llvm.org/D82485
Files:
clang/unittests/Tooling/CMakeLists.txt
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
Index: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
@@ -0,0 +1,881 @@
+//===--- clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "TestVisitor.h"
+
+using namespace clang;
+
+namespace {
+
+enum class ShouldTraversePostOrder : bool {
+ No = false,
+ Yes = true,
+};
+
+/// Base class for tests for RecursiveASTVisitor tests that validate the
+/// sequence of calls to user-defined callbacks like Traverse*(), WalkUp*(),
+/// Visit*().
+template
+class RecordingVisitorBase : public TestVisitor {
+ ShouldTraversePostOrder ShouldTraversePostOrderValue;
+
+public:
+ RecordingVisitorBase(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+ : ShouldTraversePostOrderValue(ShouldTraversePostOrderValue) {}
+
+ bool shouldTraversePostOrder() const {
+return static_cast(ShouldTraversePostOrderValue);
+ }
+
+ // Callbacks received during traversal.
+ std::string CallbackLog;
+ unsigned CallbackLogIndent = 0;
+
+ std::string stmtToString(Stmt *S) {
+StringRef ClassName = S->getStmtClassName();
+if (IntegerLiteral *IL = dyn_cast(S)) {
+ return (ClassName + "(" + IL->getValue().toString(10, false) + ")").str();
+}
+if (BinaryOperator *BO = dyn_cast(S)) {
+ return (ClassName + "(" + BinaryOperator::getOpcodeStr(BO->getOpcode()) +
+ ")")
+ .str();
+}
+if (CallExpr *CE = dyn_cast(S)) {
+ if (FunctionDecl *Callee = CE->getDirectCallee()) {
+if (Callee->getIdentifier()) {
+ return (ClassName + "(" + Callee->getName() + ")").str();
+}
+ }
+}
+if (DeclRefExpr *DRE = dyn_cast(S)) {
+ if (NamedDecl *ND = DRE->getFoundDecl()) {
+if (ND->getIdentifier()) {
+ return (ClassName + "(" + ND->getName() + ")").str();
+}
+ }
+}
+return ClassName.str();
+ }
+
+ /// Record the fact that the user-defined callback member function
+ /// \p CallbackName was called with the argument \p S. Then, record the
+ /// effects of calling the default implementation \p CallDefaultFn.
+ template
+ void recordCallback(StringRef CallbackName, Stmt *S,
+ CallDefault CallDefaultFn) {
+for (unsigned i = 0; i != CallbackLogIndent; ++i) {
+ CallbackLog += " ";
+}
+CallbackLog += (CallbackName + " " + stmtToString(S) + "\n").str();
+++CallbackLogIndent;
+CallDefaultFn();
+--CallbackLogIndent;
+ }
+};
+
+template
+::testing::AssertionResult visitorCallbackLogEqual(VisitorTy Visitor,
+ StringRef Code,
+ StringRef ExpectedLog) {
+ Visitor.runOver(Code);
+ // EXPECT_EQ shows the diff between the two strings if they are different.
+ EXPECT_EQ(ExpectedLog.trim().str(),
+StringRef(Visitor.CallbackLog).trim().str());
+ if (ExpectedLog.trim() != StringRef(Visitor.CallbackLog).trim()) {
+return ::testing::AssertionFailure();
+ }
+ return ::testing::AssertionSuccess();
+}
+
+} // namespace
+
+TEST(RecursiveASTVisitor, StmtCallbacks_TraverseLeaf) {
+ class RecordingVisitor : public RecordingVisitorBase {
+ public:
+RecordingVisitor(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+: RecordingVisitorBase(ShouldTraversePostOrderValue) {}
+
+bool TraverseIntegerLiteral(IntegerLiteral *IL) {
+ recordCallback(__func__, IL, [&, this]() {
+RecordingVisitorBase::TraverseIntegerLiteral(IL);
+ });
+ return true;
+}
+
+bool WalkUpFromStmt(Stmt *S) {
+ recordCallback(__func__, S,
+ [&, this]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+ return true;
+}
+ };
+
+ StringRef Code = R"cpp(
+void add(int, int);
+void test() {
+ 1;
+ 2 + 3;
+ add(4, 5);
+}
+)cpp";
+
+ EXPECT_TRUE(visitorCallbackLogEqual(
+ RecordingVisitor(ShouldTraversePostOrder::No), Code,
+ R"txt(
+WalkUpFromStmt CompoundStmt
+TraverseIntegerLiteral IntegerLiteral(1)
+ WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromStmt BinaryOperator(+)
+TraverseIntegerLiteral IntegerLiteral(2)
+ WalkUpFromStmt
[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces
gribozavr2 added a comment.
> Now, in all the test cases we are calling the default implementation. We are
> not surfacing that WalkUpFrom can not walk up.
I think we are. The log of callbacks called from the default implementation is
indented to the right, so we clearly see what the default implementation does,
and what the behavior would have been if we didn't call the default
implementation.
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:76
+ template
+ void recordDefaultImplementation(StringRef CallbackName,
+ CallDefault CallDefaultFn) {
eduucaldas wrote:
> We don't use CallbackName.
Removed (initially I had a different implementation).
Comment at:
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:107-110
+ recordCallback(__func__, IL);
+ recordDefaultImplementation(__func__, [&, this]() {
+RecordingVisitorBase::TraverseIntegerLiteral(IL);
+ });
eduucaldas wrote:
> I think you meant to unify `recordCallback` and `recodDefaultImplentation`
> into one, and that's why you had added this `__func__` to
> `recordDefaultImplementation`.
I was passing `__func__` to `recordDefaultImplementation` for a different
reason (I had a different output format initially), but unifying the two
functions like you're suggesting makes a lot of sense. Changed the code to do
that.
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:117
+ recordDefaultImplementation(
+ __func__, [&, this]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+ return true;
eduucaldas wrote:
> Do we need this `this`?
Yes, we're calling a method on `this` from the base class.
`RecordingVisitorBase::WalkUpFromStmt(S);` is implicitly calling a member
function, `this-> RecordingVisitorBase::WalkUpFromStmt(S);`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82485/new/
https://reviews.llvm.org/D82485
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces
gribozavr updated this revision to Diff 273834.
gribozavr added a comment.
Removed an unused argument from recordDefaultImplementation.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82485/new/
https://reviews.llvm.org/D82485
Files:
clang/unittests/Tooling/CMakeLists.txt
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
Index: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
@@ -0,0 +1,897 @@
+//===--- clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "TestVisitor.h"
+
+using namespace clang;
+
+namespace {
+
+enum class ShouldTraversePostOrder : bool {
+ No = false,
+ Yes = true,
+};
+
+/// Base class for tests for RecursiveASTVisitor tests that validate the
+/// sequence of calls to user-defined callbacks like Traverse*(), WalkUp*(),
+/// Visit*().
+template
+class RecordingVisitorBase : public TestVisitor {
+ ShouldTraversePostOrder ShouldTraversePostOrderValue;
+
+public:
+ RecordingVisitorBase(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+ : ShouldTraversePostOrderValue(ShouldTraversePostOrderValue) {}
+
+ bool shouldTraversePostOrder() const {
+return static_cast(ShouldTraversePostOrderValue);
+ }
+
+ // Callbacks received during traversal.
+ std::string CallbackLog;
+ unsigned CallbackLogIndent = 0;
+
+ std::string stmtToString(Stmt *S) {
+StringRef ClassName = S->getStmtClassName();
+if (IntegerLiteral *IL = dyn_cast(S)) {
+ return (ClassName + "(" + IL->getValue().toString(10, false) + ")").str();
+}
+if (BinaryOperator *BO = dyn_cast(S)) {
+ return (ClassName + "(" + BinaryOperator::getOpcodeStr(BO->getOpcode()) +
+ ")")
+ .str();
+}
+if (CallExpr *CE = dyn_cast(S)) {
+ if (FunctionDecl *Callee = CE->getDirectCallee()) {
+if (Callee->getIdentifier()) {
+ return (ClassName + "(" + Callee->getName() + ")").str();
+}
+ }
+}
+if (DeclRefExpr *DRE = dyn_cast(S)) {
+ if (NamedDecl *ND = DRE->getFoundDecl()) {
+if (ND->getIdentifier()) {
+ return (ClassName + "(" + ND->getName() + ")").str();
+}
+ }
+}
+return ClassName.str();
+ }
+
+ /// Record the fact that the user-defined callback member function
+ /// \p CallbackName was called with the argument \p S.
+ void recordCallback(StringRef CallbackName, Stmt *S) {
+for (unsigned i = 0; i != CallbackLogIndent; ++i) {
+ CallbackLog += " ";
+}
+CallbackLog += (CallbackName + " " + stmtToString(S) + "\n").str();
+ }
+
+ template
+ void recordDefaultImplementation(CallDefault CallDefaultFn) {
+++CallbackLogIndent;
+CallDefaultFn();
+--CallbackLogIndent;
+ }
+};
+
+template
+::testing::AssertionResult visitorCallbackLogEqual(VisitorTy Visitor,
+ StringRef Code,
+ StringRef ExpectedLog) {
+ Visitor.runOver(Code);
+ // EXPECT_EQ shows the diff between the two strings if they are different.
+ EXPECT_EQ(ExpectedLog.trim().str(),
+StringRef(Visitor.CallbackLog).trim().str());
+ if (ExpectedLog.trim() != StringRef(Visitor.CallbackLog).trim()) {
+return ::testing::AssertionFailure();
+ }
+ return ::testing::AssertionSuccess();
+}
+
+} // namespace
+
+TEST(RecursiveASTVisitor, StmtCallbacks_TraverseLeaf) {
+ class RecordingVisitor : public RecordingVisitorBase {
+ public:
+RecordingVisitor(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+: RecordingVisitorBase(ShouldTraversePostOrderValue) {}
+
+bool TraverseIntegerLiteral(IntegerLiteral *IL) {
+ recordCallback(__func__, IL);
+ recordDefaultImplementation(
+ [&, this]() { RecordingVisitorBase::TraverseIntegerLiteral(IL); });
+ return true;
+}
+
+bool WalkUpFromStmt(Stmt *S) {
+ recordCallback(__func__, S);
+ recordDefaultImplementation(
+ [&, this]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+ return true;
+}
+ };
+
+ StringRef Code = R"cpp(
+void add(int, int);
+void test() {
+ 1;
+ 2 + 3;
+ add(4, 5);
+}
+)cpp";
+
+ EXPECT_TRUE(visitorCallbackLogEqual(
+ RecordingVisitor(ShouldTraversePostOrder::No), Code,
+ R"txt(
+WalkUpFromStmt CompoundStmt
+TraverseIntegerLiteral IntegerLiteral(1)
+ WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromStmt BinaryOperator(+)
+TraverseIntegerLiteral IntegerLiteral(2)
+ WalkUpFromStmt IntegerLiteral(2)
[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces
eduucaldas added a comment.
A more general feedback.
From our conversation one of the issues was that the tests wre only surfacing
overriden methods. For instance, whenever we recorded a WalkUpFromExpr, and
thus the callback showed up in the test, we actually did **not** walk up.
Now, in all the test cases we are calling the default implementation. We are
not surfacing that WalkUpFrom **can** not walk up.
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:76
+ template
+ void recordDefaultImplementation(StringRef CallbackName,
+ CallDefault CallDefaultFn) {
We don't use CallbackName.
Comment at:
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:107-110
+ recordCallback(__func__, IL);
+ recordDefaultImplementation(__func__, [&, this]() {
+RecordingVisitorBase::TraverseIntegerLiteral(IL);
+ });
I think you meant to unify `recordCallback` and `recodDefaultImplentation` into
one, and that's why you had added this `__func__` to
`recordDefaultImplementation`.
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:117
+ recordDefaultImplementation(
+ __func__, [&, this]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+ return true;
Do we need this `this`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82485/new/
https://reviews.llvm.org/D82485
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces
gribozavr updated this revision to Diff 273464.
gribozavr added a comment.
Added calls to default implementations.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82485/new/
https://reviews.llvm.org/D82485
Files:
clang/unittests/Tooling/CMakeLists.txt
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
Index: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
@@ -0,0 +1,910 @@
+//===--- clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "TestVisitor.h"
+
+using namespace clang;
+
+namespace {
+
+enum class ShouldTraversePostOrder : bool {
+ No = false,
+ Yes = true,
+};
+
+/// Base class for tests for RecursiveASTVisitor tests that validate the
+/// sequence of calls to user-defined callbacks like Traverse*(), WalkUp*(),
+/// Visit*().
+template
+class RecordingVisitorBase : public TestVisitor {
+ ShouldTraversePostOrder ShouldTraversePostOrderValue;
+
+public:
+ RecordingVisitorBase(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+ : ShouldTraversePostOrderValue(ShouldTraversePostOrderValue) {}
+
+ bool shouldTraversePostOrder() const {
+return static_cast(ShouldTraversePostOrderValue);
+ }
+
+ // Callbacks received during traversal.
+ std::string CallbackLog;
+ unsigned CallbackLogIndent = 0;
+
+ std::string stmtToString(Stmt *S) {
+StringRef ClassName = S->getStmtClassName();
+if (IntegerLiteral *IL = dyn_cast(S)) {
+ return (ClassName + "(" + IL->getValue().toString(10, false) + ")").str();
+}
+if (BinaryOperator *BO = dyn_cast(S)) {
+ return (ClassName + "(" + BinaryOperator::getOpcodeStr(BO->getOpcode()) +
+ ")")
+ .str();
+}
+if (CallExpr *CE = dyn_cast(S)) {
+ if (FunctionDecl *Callee = CE->getDirectCallee()) {
+if (Callee->getIdentifier()) {
+ return (ClassName + "(" + Callee->getName() + ")").str();
+}
+ }
+}
+if (DeclRefExpr *DRE = dyn_cast(S)) {
+ if (NamedDecl *ND = DRE->getFoundDecl()) {
+if (ND->getIdentifier()) {
+ return (ClassName + "(" + ND->getName() + ")").str();
+}
+ }
+}
+return ClassName.str();
+ }
+
+ /// Record the fact that the user-defined callback member function
+ /// \p CallbackName was called with the argument \p S.
+ void recordCallback(StringRef CallbackName, Stmt *S) {
+for (unsigned i = 0; i != CallbackLogIndent; ++i) {
+ CallbackLog += " ";
+}
+CallbackLog += (CallbackName + " " + stmtToString(S) + "\n").str();
+ }
+
+ template
+ void recordDefaultImplementation(StringRef CallbackName,
+ CallDefault CallDefaultFn) {
+++CallbackLogIndent;
+CallDefaultFn();
+--CallbackLogIndent;
+ }
+};
+
+template
+::testing::AssertionResult visitorCallbackLogEqual(VisitorTy Visitor,
+ StringRef Code,
+ StringRef ExpectedLog) {
+ Visitor.runOver(Code);
+ // EXPECT_EQ shows the diff between the two strings if they are different.
+ EXPECT_EQ(ExpectedLog.trim().str(),
+StringRef(Visitor.CallbackLog).trim().str());
+ if (ExpectedLog.trim() != StringRef(Visitor.CallbackLog).trim()) {
+return ::testing::AssertionFailure();
+ }
+ return ::testing::AssertionSuccess();
+}
+
+} // namespace
+
+TEST(RecursiveASTVisitor, StmtCallbacks_TraverseLeaf) {
+ class RecordingVisitor : public RecordingVisitorBase {
+ public:
+RecordingVisitor(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+: RecordingVisitorBase(ShouldTraversePostOrderValue) {}
+
+bool TraverseIntegerLiteral(IntegerLiteral *IL) {
+ recordCallback(__func__, IL);
+ recordDefaultImplementation(__func__, [&, this]() {
+RecordingVisitorBase::TraverseIntegerLiteral(IL);
+ });
+ return true;
+}
+
+bool WalkUpFromStmt(Stmt *S) {
+ recordCallback(__func__, S);
+ recordDefaultImplementation(
+ __func__, [&, this]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+ return true;
+}
+ };
+
+ StringRef Code = R"cpp(
+void add(int, int);
+void test() {
+ 1;
+ 2 + 3;
+ add(4, 5);
+}
+)cpp";
+
+ EXPECT_TRUE(visitorCallbackLogEqual(
+ RecordingVisitor(ShouldTraversePostOrder::No), Code,
+ R"txt(
+WalkUpFromStmt CompoundStmt
+TraverseIntegerLiteral IntegerLiteral(1)
+ WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromStmt BinaryOperator(+)
[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces
gribozavr updated this revision to Diff 273302.
gribozavr added a comment.
Addressed code review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82485/new/
https://reviews.llvm.org/D82485
Files:
clang/unittests/Tooling/CMakeLists.txt
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
Index: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
@@ -0,0 +1,644 @@
+//===--- clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "TestVisitor.h"
+
+using namespace clang;
+
+namespace {
+
+enum class ShouldTraversePostOrder : bool {
+ No = false,
+ Yes = true,
+};
+
+/// Base class for tests for RecursiveASTVisitor tests that validate the
+/// sequence of calls to user-defined callbacks like Traverse*(), WalkUp*(),
+/// Visit*().
+template
+class RecordingVisitorBase : public TestVisitor {
+ ShouldTraversePostOrder ShouldTraversePostOrderValue;
+
+public:
+ RecordingVisitorBase(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+ : ShouldTraversePostOrderValue(ShouldTraversePostOrderValue) {}
+
+ bool shouldTraversePostOrder() const {
+return static_cast(ShouldTraversePostOrderValue);
+ }
+
+ // Callbacks received during traversal.
+ std::string CallbackLog;
+
+ std::string stmtToString(Stmt *S) {
+StringRef ClassName = S->getStmtClassName();
+if (IntegerLiteral *IL = dyn_cast(S)) {
+ return (ClassName + "(" + IL->getValue().toString(10, false) + ")").str();
+}
+if (BinaryOperator *BO = dyn_cast(S)) {
+ return (ClassName + "(" + BinaryOperator::getOpcodeStr(BO->getOpcode()) +
+ ")")
+ .str();
+}
+if (CallExpr *CE = dyn_cast(S)) {
+ if (FunctionDecl *Callee = CE->getDirectCallee()) {
+if (Callee->getIdentifier()) {
+ return (ClassName + "(" + Callee->getName() + ")").str();
+}
+ }
+}
+if (DeclRefExpr *DRE = dyn_cast(S)) {
+ if (NamedDecl *ND = DRE->getFoundDecl()) {
+if (ND->getIdentifier()) {
+ return (ClassName + "(" + ND->getName() + ")").str();
+}
+ }
+}
+return ClassName.str();
+ }
+
+ /// Record the fact that the user-defined callback member function
+ /// \p CallbackName was called with the argument \p S.
+ void recordCallback(StringRef CallbackName, Stmt *S) {
+CallbackLog += (CallbackName + " " + stmtToString(S) + "\n").str();
+ }
+};
+
+template
+::testing::AssertionResult visitorCallbackLogEqual(VisitorTy Visitor,
+ StringRef Code,
+ StringRef ExpectedLog) {
+ Visitor.runOver(Code);
+ // EXPECT_EQ shows the diff between the two strings if they are different.
+ EXPECT_EQ(ExpectedLog.trim().str(),
+StringRef(Visitor.CallbackLog).trim().str());
+ if (ExpectedLog.trim() != StringRef(Visitor.CallbackLog).trim()) {
+return ::testing::AssertionFailure();
+ }
+ return ::testing::AssertionSuccess();
+}
+
+} // namespace
+
+TEST(RecursiveASTVisitor, Callbacks_TraverseLeaf) {
+ class RecordingVisitor : public RecordingVisitorBase {
+ public:
+RecordingVisitor(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+: RecordingVisitorBase(ShouldTraversePostOrderValue) {}
+
+bool TraverseIntegerLiteral(IntegerLiteral *IL) {
+ recordCallback(__func__, IL);
+ return true;
+}
+
+bool WalkUpFromStmt(Stmt *S) {
+ recordCallback(__func__, S);
+ return true;
+}
+ };
+
+ StringRef Code = R"cpp(
+void add(int, int);
+void test() {
+ 1;
+ 2 + 3;
+ add(4, 5);
+}
+)cpp";
+
+ EXPECT_TRUE(visitorCallbackLogEqual(
+ RecordingVisitor(ShouldTraversePostOrder::No), Code,
+ R"txt(
+WalkUpFromStmt CompoundStmt
+TraverseIntegerLiteral IntegerLiteral(1)
+WalkUpFromStmt BinaryOperator(+)
+TraverseIntegerLiteral IntegerLiteral(2)
+TraverseIntegerLiteral IntegerLiteral(3)
+WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt ImplicitCastExpr
+WalkUpFromStmt DeclRefExpr(add)
+TraverseIntegerLiteral IntegerLiteral(4)
+TraverseIntegerLiteral IntegerLiteral(5)
+)txt"));
+
+ EXPECT_TRUE(visitorCallbackLogEqual(
+ RecordingVisitor(ShouldTraversePostOrder::Yes), Code,
+ R"txt(
+TraverseIntegerLiteral IntegerLiteral(1)
+WalkUpFromStmt IntegerLiteral(1)
+TraverseIntegerLiteral IntegerLiteral(2)
+WalkUpFromStmt IntegerLiteral(2)
+TraverseIntegerLiteral IntegerLiteral(3)
+WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromStmt BinaryOperator(+)
[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces
gribozavr2 marked 5 inline comments as done.
gribozavr2 added inline comments.
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:16
+template
+class RecordingVisitorBase : public TestVisitor {
+ bool VisitPostOrder;
ymandel wrote:
> Add class comment?
Added.
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:17
+class RecordingVisitorBase : public TestVisitor {
+ bool VisitPostOrder;
+
eduucaldas wrote:
> ymandel wrote:
> > Consider using an enum rather than a bool.
> Agreed.
> This would avoid all the /*VisitPostOrder=*/false
>
Changed to enum.
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:81
+
+bool TraverseIntegerLiteral(IntegerLiteral *E) {
+ recordCallback(__func__, E);
eduucaldas wrote:
> E for Expr? Ok, Expr is base class of IntegerLiteral.
> I propose to use either:
> S for Stmt, it is a more homogeneus name and **also** a base class of
> IntegerLiteral
> Or
> IL for IntegerLiteral, and then we stick with this convention
Changed to a more specific abbreviation everywhere I could find.
Comment at:
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:148-161
+bool WalkUpFromStmt(Stmt *S) {
+ recordCallback(__func__, S);
+ return true;
+}
+
+bool WalkUpFromExpr(Expr *E) {
+ recordCallback(__func__, E);
eduucaldas wrote:
> > E for Expr? Ok, Expr is base class of IntegerLiteral.
> > I propose to use either:
> > S for Stmt, it is a more homogeneus name and also a base class of
> > IntegerLiteral
> > Or
> > IL for IntegerLiteral, and then we stick with this convention
>
> Here it gets even more confusing.
Changed the name to `IL`.
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:153
+
+bool WalkUpFromExpr(Expr *E) {
+ recordCallback(__func__, E);
eduucaldas wrote:
> I think overriding WalkUpFromDerived when you already have WalkUpFromStmt
> doesn't bring much value.
> In the case of fully derived AST nodes we just get the repeated information
> about the type of this node, e.g.
> WalkUpFromIntegerLiteral IntegerLiteral(x)
> instead of
> WalkUpFromStmt IntegerLiteral(x)
>
> In the case of intermediate AST nodes, as WalkUpFromExpr, we get some
> information but do we need that?
> Here for instance, the information we get is:
> WalkUpFromExpr Node => Node is an Expr
> WalkUpFromStmt Node => Node is a Stmt
> I don't think this information is very valuable
I added these overrides not to collect more information, but to get more
coverage. It is true that if we look carefully at the implementation we can
probably infer that WalkUpFrom chaining works fine. I'm adding tests to ensure
that it continues to work correctly in future. It would be nice if we could
factor out a separate test that shows just that chaining logic, but I don't see
how to easily do it. WalkUpFrom methods are called from a bunch of places
depending on what Traverse methods are overridden, whether we are in the data
recursion optimization, and whether we are in the post-order traversal mode.
Those focused tests would have to define the same combinations of callbacks in
RecursiveASTVisitor as these tests here.
Comment at:
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:300-309
+WalkUpFromStmt CompoundStmt
+WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromStmt BinaryOperator(+)
+WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt ImplicitCastExpr
eduucaldas wrote:
> I think we spotted something funny here. RAV didn't call our overriden
> TraverseBinaryOperator.
I added a FIXME that explains that this is a potential bug.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82485/new/
https://reviews.llvm.org/D82485
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces
eduucaldas marked 2 inline comments as done.
eduucaldas added inline comments.
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:1
+//===--- clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
---===//
+//
I find this name too general.
We are testing here the behavior of TraverseStmt specifically, perhaps
`TraverseStmt.cpp` would be a more appropriate name
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:17
+class RecordingVisitorBase : public TestVisitor {
+ bool VisitPostOrder;
+
ymandel wrote:
> Consider using an enum rather than a bool.
Agreed.
This would avoid all the /*VisitPostOrder=*/false
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:81
+
+bool TraverseIntegerLiteral(IntegerLiteral *E) {
+ recordCallback(__func__, E);
E for Expr? Ok, Expr is base class of IntegerLiteral.
I propose to use either:
S for Stmt, it is a more homogeneus name and **also** a base class of
IntegerLiteral
Or
IL for IntegerLiteral, and then we stick with this convention
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:124
+TraverseIntegerLiteral IntegerLiteral(3)
+WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromStmt BinaryOperator(+)
Good! Here the post order is still calling WalkUpFrom even though our Traverse
doesn't call it in IntegerLiteral!
Comment at:
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:148-161
+bool WalkUpFromStmt(Stmt *S) {
+ recordCallback(__func__, S);
+ return true;
+}
+
+bool WalkUpFromExpr(Expr *E) {
+ recordCallback(__func__, E);
> E for Expr? Ok, Expr is base class of IntegerLiteral.
> I propose to use either:
> S for Stmt, it is a more homogeneus name and also a base class of
> IntegerLiteral
> Or
> IL for IntegerLiteral, and then we stick with this convention
Here it gets even more confusing.
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:153
+
+bool WalkUpFromExpr(Expr *E) {
+ recordCallback(__func__, E);
I think overriding WalkUpFromDerived when you already have WalkUpFromStmt
doesn't bring much value.
In the case of fully derived AST nodes we just get the repeated information
about the type of this node, e.g.
WalkUpFromIntegerLiteral IntegerLiteral(x)
instead of
WalkUpFromStmt IntegerLiteral(x)
In the case of intermediate AST nodes, as WalkUpFromExpr, we get some
information but do we need that?
Here for instance, the information we get is:
WalkUpFromExpr Node => Node is an Expr
WalkUpFromStmt Node => Node is a Stmt
I don't think this information is very valuable
Comment at:
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:300-309
+WalkUpFromStmt CompoundStmt
+WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromStmt BinaryOperator(+)
+WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt ImplicitCastExpr
I think we spotted something funny here. RAV didn't call our overriden
TraverseBinaryOperator.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82485/new/
https://reviews.llvm.org/D82485
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces
ymandel accepted this revision.
ymandel added inline comments.
This revision is now accepted and ready to land.
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:16
+template
+class RecordingVisitorBase : public TestVisitor {
+ bool VisitPostOrder;
Add class comment?
Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp:17
+class RecordingVisitorBase : public TestVisitor {
+ bool VisitPostOrder;
+
Consider using an enum rather than a bool.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82485/new/
https://reviews.llvm.org/D82485
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces
gribozavr created this revision.
Herald added subscribers: cfe-commits, mgorny.
Herald added a project: clang.
gribozavr2 added reviewers: ymandel, eduucaldas.
These tests show a bug: post-order traversal introduces an extra call to
WalkUp*, that is not present in pre-order traversal. I'm fixing this bug
in a follow-up commit.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D82485
Files:
clang/unittests/Tooling/CMakeLists.txt
clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
Index: clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp
@@ -0,0 +1,628 @@
+//===--- clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "TestVisitor.h"
+
+using namespace clang;
+
+namespace {
+
+template
+class RecordingVisitorBase : public TestVisitor {
+ bool VisitPostOrder;
+
+public:
+ RecordingVisitorBase(bool VisitPostOrder) : VisitPostOrder(VisitPostOrder) {}
+
+ bool shouldTraversePostOrder() const { return VisitPostOrder; }
+
+ // Callbacks received during traversal.
+ std::string CallbackLog;
+
+ std::string stmtToString(Stmt *S) {
+StringRef ClassName = S->getStmtClassName();
+if (IntegerLiteral *IL = dyn_cast(S)) {
+ return (ClassName + "(" + IL->getValue().toString(10, false) + ")").str();
+}
+if (BinaryOperator *BO = dyn_cast(S)) {
+ return (ClassName + "(" + BinaryOperator::getOpcodeStr(BO->getOpcode()) +
+ ")")
+ .str();
+}
+if (CallExpr *CE = dyn_cast(S)) {
+ if (FunctionDecl *Callee = CE->getDirectCallee()) {
+if (Callee->getIdentifier()) {
+ return (ClassName + "(" + Callee->getName() + ")").str();
+}
+ }
+}
+if (DeclRefExpr *DRE = dyn_cast(S)) {
+ if (NamedDecl *ND = DRE->getFoundDecl()) {
+if (ND->getIdentifier()) {
+ return (ClassName + "(" + ND->getName() + ")").str();
+}
+ }
+}
+return ClassName.str();
+ }
+
+ void recordCallback(StringRef CallbackName, Stmt *S) {
+CallbackLog += (CallbackName + " " + stmtToString(S) + "\n").str();
+ }
+};
+
+template
+::testing::AssertionResult visitorCallbackLogEqual(VisitorTy Visitor,
+ StringRef Code,
+ StringRef ExpectedLog) {
+ Visitor.runOver(Code);
+ // EXPECT_EQ shows the diff between the two strings if they are different.
+ EXPECT_EQ(ExpectedLog.trim().str(),
+StringRef(Visitor.CallbackLog).trim().str());
+ if (ExpectedLog.trim() != StringRef(Visitor.CallbackLog).trim()) {
+return ::testing::AssertionFailure();
+ }
+ return ::testing::AssertionSuccess();
+}
+
+} // namespace
+
+TEST(RecursiveASTVisitor, Callbacks_TraverseLeaf) {
+ class RecordingVisitor : public RecordingVisitorBase {
+ public:
+RecordingVisitor(bool VisitPostOrder)
+: RecordingVisitorBase(VisitPostOrder) {}
+
+bool TraverseIntegerLiteral(IntegerLiteral *E) {
+ recordCallback(__func__, E);
+ return true;
+}
+
+bool WalkUpFromStmt(Stmt *S) {
+ recordCallback(__func__, S);
+ return true;
+}
+ };
+
+ StringRef Code = R"cpp(
+void add(int, int);
+void test() {
+ 1;
+ 2 + 3;
+ add(4, 5);
+}
+)cpp";
+
+ EXPECT_TRUE(
+ visitorCallbackLogEqual(RecordingVisitor(/*VisitPostOrder=*/false), Code,
+ R"txt(
+WalkUpFromStmt CompoundStmt
+TraverseIntegerLiteral IntegerLiteral(1)
+WalkUpFromStmt BinaryOperator(+)
+TraverseIntegerLiteral IntegerLiteral(2)
+TraverseIntegerLiteral IntegerLiteral(3)
+WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt ImplicitCastExpr
+WalkUpFromStmt DeclRefExpr(add)
+TraverseIntegerLiteral IntegerLiteral(4)
+TraverseIntegerLiteral IntegerLiteral(5)
+)txt"));
+
+ EXPECT_TRUE(visitorCallbackLogEqual(RecordingVisitor(/*VisitPostOrder=*/true),
+ Code,
+ R"txt(
+TraverseIntegerLiteral IntegerLiteral(1)
+WalkUpFromStmt IntegerLiteral(1)
+TraverseIntegerLiteral IntegerLiteral(2)
+WalkUpFromStmt IntegerLiteral(2)
+TraverseIntegerLiteral IntegerLiteral(3)
+WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromStmt BinaryOperator(+)
+WalkUpFromStmt DeclRefExpr(add)
+WalkUpFromStmt ImplicitCastExpr
+TraverseIntegerLiteral IntegerLiteral(4)
+WalkUpFromStmt IntegerLiteral(4)
+TraverseIntegerLiteral IntegerLiteral(5)
+WalkUpFromStmt IntegerLiteral(5)
+WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt CompoundStmt
+)txt"));
+}
+
+TEST(RecursiveASTVisitor,
