[PATCH] D85295: [SyntaxTree] Implement the List construct.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

I feel uneasy about adding this code without tests. Could we maybe port the function parameter list to use this infrastructure, and then add tests that exercise `getElementsAsNodesAndDelimiters`?



================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:194
 
+enum TerminationKind {
+  Terminated,
----------------
`enum class`?

Also, I think it should be renamed to `ListTerminationKind` or moved into `List` as a nested type.


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:197
+  MaybeTerminated,
+  Separated,
+};
----------------
Add a "WithoutDelimiters" case as well?


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:200
+
+class List : public Tree {
+  template <typename Element> struct ElementAndDelimiter {
----------------
Could you add a doc comment?


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:229
+  /// elements to empty or one-element lists.
+  clang::tok::TokenKind getDelimiterTokenKind();
+
----------------
Should we change this function to return optional to allow representing lists that don't have any delimiters?


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:232
+  TerminationKind getTerminationKind();
+  bool canBeEmpty();
+};
----------------
Please add a doc comment that emphasizes that any list can be empty when the syntax tree represents code with syntax errors.


================
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:277
+  if (!firstChild()) {
+    assert(canBeEmpty());
+    return {};
----------------
This assert is only correct for valid code. When a syntax tree represents invalid code, any list can be empty.


================
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:293
+      children.push_back(
+          {elementWithoutDelimiter, llvm::cast<syntax::Leaf>(C)});
+      elementWithoutDelimiter = nullptr;
----------------



================
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:298
+    default:
+      llvm_unreachable("A list has only elements or delimiters.");
+    }
----------------



================
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:368
+clang::tok::TokenKind syntax::List::getDelimiterTokenKind() {
+  return clang::tok::TokenKind::unknown;
+}
----------------
I think the eventual implementation should switch on the syntax tree node kind and return the appropriate information. Right now, there are no subclasses of List, to these methods should be implemented as llvm_unreachable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85295