Remove the SyntaxData abstraction layer #2129
Conversation
|
@swift-ci Please test |
CodeGeneration/Sources/generate-swift-syntax/templates/swiftsyntax/SyntaxBaseNodesFile.swift
Show resolved
Hide resolved
CodeGeneration/Sources/generate-swift-syntax/templates/swiftsyntax/SyntaxBaseNodesFile.swift
Show resolved
Hide resolved
| return self.firstToken(viewMode: .sourceAccurate) | ||
| } | ||
|
|
||
| /// Returns the first token node that is part of this syntax node. |
There was a problem hiding this comment.
Nit: unrelated to this PR, would be good to document parameters and what viewMode does.
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| struct AbsoluteRawSyntax { |
There was a problem hiding this comment.
Nit: unrelated to this PR. It would be good to add more docs on what AbsoluteRawSyntax does. (Also may just be because I'm a beginner and didn't have enough exposure yet, but that's probably who the documentation is for).
|
@natikgadzhi You are right about the documentation. I’d prefer to keep these changes out of this PR though because it’s already quite big. |
ahoppen
force-pushed
the
ahoppen/kill-syntaxdata
branch
from
4608340 to
a2e4a3b
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
| // MARK: Type information | ||
|
|
||
| extension SyntaxProtocol { | ||
| /// The kind of the syntax node, e.g. if it is a `functionDecl`. |
There was a problem hiding this comment.
Would it make sense to refer to the SyntaxKind.functionDecl?
Just so the developer can navigate to the actual type?
There was a problem hiding this comment.
Good idea. I just copied that comment but might as well improve it.
| // MARK: Children / parent | ||
|
|
||
| public extension SyntaxProtocol { | ||
| /// A sequence over the `present` children of this node. |
There was a problem hiding this comment.
I'm not sure I understand this doc comment.
Does that mean, not matter the view mode you provide, then it's only the present children of the node, that will be returned
There was a problem hiding this comment.
Oh, that comment still pre-dated the introduction of SyntaxTreeViewMode. I removed the present part of the comment.
| } | ||
| } | ||
|
|
||
| /// AbsoluteSyntaxInfo represents the information that relates a RawSyntax to a |
There was a problem hiding this comment.
Is there missing some ticks around RawSyntax?
| /// different. For example two different ``FunctionDeclSyntax`` nodes in the | ||
| /// might have the exact same contents but if they occur at a different |
There was a problem hiding this comment.
"... nodes in the might have" it sounds a bit strange to me?
| let rootId: UInt | ||
| /// Unique value for a node within its own tree. |
There was a problem hiding this comment.
Some places do we have a newline after declarations. Personally I think it's easier to read.
What do you think of adding new lines after a declaration ?
ahoppen
force-pushed
the
ahoppen/kill-syntaxdata
branch
from
0ec0cf2 to
93f69c2
Compare
|
@swift-ci Please test |
|
@swift-ci Please test macOS |
|
@swift-ci Please test Windows |
`SyntaxData` just held the data of the `Syntax` node and didn’t provide any additional functionality. Instead, it required unecessary casts between `Syntax` and `SyntaxData` and we were inconsistent about where we took `Syntax` or `SyntaxData`. Store the members of `SyntaxData` directly in `Syntax`.
ahoppen
force-pushed
the
ahoppen/kill-syntaxdata
branch
from
93f69c2 to
e8e60fb
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
SyntaxDatajust held the data of theSyntaxnode and didn’t provide any additional functionality. Instead, it required unecessary casts betweenSyntaxandSyntaxDataand we were inconsistent about where we tookSyntaxorSyntaxData. Store the members ofSyntaxDatadirectly inSyntax.I saw slight (0.5% - 2%) performance improvements in instruction count when running swift-format with this change.
The best way to review this is commit-by-commit. The first two commits just split up files, the last commit is the actual removal of
SyntaxData.