We defined a List construct to help with the implementation of list-like
grammar rules. This is a first implementation of this API.
Details
- Reviewers
gribozavr2 - Commits
- rGa90c78ac5261: [SyntaxTree] Implement the List construct.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/include/clang/Tooling/Syntax/Tree.h | ||
---|---|---|
223 | As this is a base List, we don't have the necessary information tom implement the following methods. I chose to give them default values, check their definition |
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?
clang/include/clang/Tooling/Syntax/Tree.h | ||
---|---|---|
194 | enum class? Also, I think it should be renamed to ListTerminationKind or moved into List as a nested type. | |
197 | Add a "WithoutDelimiters" case as well? | |
200 | Could you add a doc comment? | |
229 | Should we change this function to return optional to allow representing lists that don't have any delimiters? | |
232 | Please add a doc comment that emphasizes that any list can be empty when the syntax tree represents code with syntax errors. | |
clang/lib/Tooling/Syntax/Tree.cpp | ||
275 | This assert is only correct for valid code. When a syntax tree represents invalid code, any list can be empty. | |
291 | ||
296 | ||
366 | 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. |
Answer comments
non-delimited-lists need further discussion
clang/include/clang/Tooling/Syntax/Tree.h | ||
---|---|---|
197 | I think we might want to treat non-delimited-list in another way. as many of the member functions stop making sense. | |
229 | Cf, previous comment on non-delimited-lists | |
clang/lib/Tooling/Syntax/Tree.cpp | ||
275 | Yeah, of course, we had already discussed that verifying those things in the casa of valid code would be the task of the verifier. |
clang/include/clang/Tooling/Syntax/Tree.h | ||
---|---|---|
197 | OK, I'm convinced. |
clang/include/clang/Tooling/Syntax/Tree.h | ||
---|---|---|
247 | I just realized that the rest of the syntax tree API does not use the get~ prefix. However, most of Clang does, as far as I know. Which way should we repaint? |
clang/include/clang/Tooling/Syntax/Tree.h | ||
---|---|---|
247 | Let's go with Clang! I can make this change in another patch |
clang/include/clang/Tooling/Syntax/Tree.h | ||
---|---|---|
194 | """ This type models the following grammar construct: | |
249 | "Whether this list can be empty in syntactically and semantically correct code. This list may be empty when the source code has errors even if canBeEmpty() returns true." | |
clang/lib/Tooling/Syntax/Tree.cpp | ||
365 | I think a better error message would say that there are no subclasses of List, so this method can't be called. |
enum class?
Also, I think it should be renamed to ListTerminationKind or moved into List as a nested type.