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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
