[PATCH] D85962: [SyntaxTree] Create annotations infrastructure and apply it in expression tests.

[Eduardo Caldas via Phabricator via cfe-commits]

eduucaldas added inline comments.


================
Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:477
 struct X {
   friend X operator+(const X&, const X&);
 };
----------------
A test for this part was created below


================
Comment at: clang/unittests/Tooling/Syntax/TreeTestBase.cpp:199
+  auto AnnotatedRanges = AnnotatedCode.ranges();
+  assert(AnnotatedRanges.size() == TreeDumps.size());
+  for (auto i = 0ul; i < AnnotatedRanges.size(); i++) {
----------------
gribozavr2 wrote:
> ASSERT_EQ I think would be better.
`ASSERT_EQ` is a macro that returns void, so we cannot use it here.

However that brings another question.
Right now we have methods `treeDumpEqual*` that return `AssertionResult`s and we use them in our tests in the following way: `EXPECT_TRUE(treeDumpEqual*...)`.
It seems to me that we should instead perform any assertion inside `treeDumpEqual*`, and then just call it directly in the test.

WDYT?  I case you agree we can perform this change in another patch.


================
Comment at: clang/unittests/Tooling/Syntax/TreeTestBase.cpp:206
+    // EXPECT_EQ shows the diff between the two strings if they are different.
+    EXPECT_EQ(TreeDumps[i].trim().str(), AnnotatedNodeDump);
+    if (AnnotatedNodeDump != TreeDumps[i].trim().str())
----------------
gribozavr2 wrote:
> Could you dump the annotated source code substring to make debugging failing tests easier?
Very good suggestion :) thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85962/new/

https://reviews.llvm.org/D85962