[PATCH] D87839: [SyntaxTree] Test the List API

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:224
+  /// "a, b  c"  <=> [("a", ","), ("b", nul), ("c", nul)]
+  /// "a, b,"    <=> [("a", ","), ("b", ","), (nul, nul)]
   ///
----------------
I'd slightly prefer "null" b/c "nul" refers to the ASCII character. Feel free to add more spaces to make columns line up :)


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:129
+  std::string dump(const List::ElementAndDelimiter<Node> &ED) {
+    auto Format = [](StringRef s) { return "'" + s.trim().str() + "'"; };
+
----------------



================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:154
+      OS << ", " << dump(*ED);
+    OS << "]";
+    return OS.str();
----------------
Would llvm::interleaveComma help?


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:330
+            "[('a', '::'), ('b', '::'), ('c', nul)]");
+}
+
----------------
All of the tests above are testing getElementsAsNodesAndDelimiters -- could you put that into the test names? Or do you plan adding more assertions in the end? Or is that the only meaningful assertion? (I think not, we could also test the low-level non-typed function at least.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87839