This is an archive of the discontinued LLVM Phabricator instance.

[SyntaxTree] Implement the List construct.
ClosedPublic

Authored by eduucaldas on Aug 5 2020, 5:09 AM.

Details

Summary

We defined a List construct to help with the implementation of list-like
grammar rules. This is a first implementation of this API.

Diff Detail

Event Timeline

eduucaldas created this revision.Aug 5 2020, 5:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2020, 5:09 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
eduucaldas requested review of this revision.Aug 5 2020, 5:09 AM
eduucaldas added inline comments.
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
277

This assert is only correct for valid code. When a syntax tree represents invalid code, any list can be empty.

293
298
368

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.

eduucaldas updated this revision to Diff 283536.Aug 6 2020, 2:17 AM
eduucaldas marked 9 inline comments as done.

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.
getElementsAsNodesAndDelimiters
getDelimiterTokenKind
getTerminationKind

229

Cf, previous comment on non-delimited-lists

clang/lib/Tooling/Syntax/Tree.cpp
277

Yeah, of course, we had already discussed that verifying those things in the casa of valid code would be the task of the verifier.

gribozavr2 added inline comments.Aug 6 2020, 6:05 AM
clang/include/clang/Tooling/Syntax/Tree.h
197

OK, I'm convinced.

gribozavr2 added inline comments.Aug 6 2020, 10:05 AM
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?

eduucaldas marked an inline comment as done.Aug 6 2020, 10:42 AM
eduucaldas added inline comments.
clang/include/clang/Tooling/Syntax/Tree.h
247

Let's go with Clang! I can make this change in another patch

eduucaldas marked an inline comment as done.Aug 6 2020, 10:44 AM
gribozavr2 accepted this revision.Aug 10 2020, 1:17 AM
gribozavr2 added inline comments.
clang/include/clang/Tooling/Syntax/Tree.h
194

"""
A list of elements separated or terminated by a fixed token.

This type models the following grammar construct:
delimited-list(element, delimiter, termination, canBeEmpty)
"""

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
367

I think a better error message would say that there are no subclasses of List, so this method can't be called.

This revision is now accepted and ready to land.Aug 10 2020, 1:17 AM
eduucaldas marked 3 inline comments as done.

Answer code-review

clang/include/clang/Tooling/Syntax/Tree.h
249

"...even if canBeEmpty() returns false"

clang/lib/Tooling/Syntax/Tree.cpp
367

Very sorry about that, this error was a copy-paste, I forgot to edit it

gribozavr2 accepted this revision.Aug 10 2020, 3:09 AM
This revision was landed with ongoing or failed builds.Aug 10 2020, 3:32 AM
This revision was automatically updated to reflect the committed changes.