Reimplement SyntaxVisitor to improve visiting performance. #32
Conversation
|
@ahoppen does this approach make sense to you? |
nkcsgexi
force-pushed
the
wip-visitor
branch
2 times, most recently
from
71e749e to
919983b
Compare
There was a problem hiding this comment.
Great PR overall. I knew this was a bottleneck for quite some time. Thanks for tackling this. I would like to get rid of childIdxChain and nodeChain and represent those by a recursive enum which I believe would make the design a lot easier to understand. Comments for those inline
| class RawSyntaxVisitor { | ||
| private let visitor: SyntaxVisitor | ||
| private var childIdxChain: [Int] | ||
| private var nodeChain: [Syntax] |
There was a problem hiding this comment.
I don't really like the idea of having those two data structures that together form the path to the current node. Couldn't we define an enum like this (names may be changed)
enum SyntaxTreeNode {
/// We already have a `Syntax` node realised for this node
case realized(node: Syntax)
/// This node does not have a `Syntax` node instantiated yet. If needed, we need to compute it from its parent RawSyntax node
case virtual(parent: SyntaxTreePathNode, index: Int)
}Then have one private var currentNode: SyntaxTreeNode. Then, when moving down, we just need to wrap the current currentNode in one more level of virtual, moving up needs to be a switch on the current currentNode. If it is a virtual node, we just need to reference its parent, if it is realized, we can use node.parent.
Realising a node is then just a recursive operation on SyntaxTreeNode:
enum SyntaxTreeNode {
// …
func realize() -> Syntax {
switch self {
case .realized(let node):
return node
case .virtual(let parent, let index):
return parent.realized().child(at: index)
}
}
}I think that should make the entire visitor a lot simpler to understand.
| childIdxChain.removeAll(keepingCapacity: true) | ||
| let node = nodeChain.last! | ||
| visitor.visitPre(node) | ||
| visitor.visit(node) |
There was a problem hiding this comment.
I would like to either have the responsibility of calling visitPre and calling visitPost in RawSyntax or RawSyntaxVisitor, not spread across them. If it's not possible to call visitPost in here (which I think it isn't), I would also move the call to visitPre to RawSyntax.accept.
| self.childIdxChain = [] | ||
| } | ||
|
|
||
| convenience init(_ another: RawSyntaxVisitor, _ root: Syntax) { |
There was a problem hiding this comment.
Do we need this constructor anywhere?
Sources/SwiftSyntax/RawSyntax.swift
Outdated
| case .node(let kind,let layout): | ||
| let shouldVisit = visitor.shouldVisit(kind) | ||
| if shouldVisit { | ||
|
|
There was a problem hiding this comment.
Nitpick: There is a spare empty line here for my taste
nkcsgexi
force-pushed
the
wip-visitor
branch
from
919983b to
8a62bd3
Compare
This implementation allows users to specify interested SyntaxNodeKind and TokenKind; thus we can traverse the RawSyntax Tree instead of SyntaxTree and realize only the interested RawSyntax nodes into SyntaxNodes.
nkcsgexi
force-pushed
the
wip-visitor
branch
from
7c2bc5f to
f174c4d
Compare
|
@swift-ci please test |
|
Build failed |
| @@ -98,12 +98,23 @@ open class SyntaxVisitor { | |||
| public init() {} | |||
| % for node in SYNTAX_NODES: | |||
| % if is_visitable(node): | |||
| open func visit(_ node: ${node.name}) { | |||
| visitChildren(node) | |||
| open func visit(_ node: ${node.name}) -> Bool { | |||
There was a problem hiding this comment.
Can we add a doc comment specifying what the return value does? And: Do we ever return false here?
| /// A wrapper over Syntax. A syntax node is only realized when explicitly asked; | ||
| /// otherwise the node is represented as a child index list from a realized | ||
| /// ancestor. | ||
| class PendingSyntaxNode { |
There was a problem hiding this comment.
Can’t we just have one indirect enum instead of the class wrapping the enum?
| open func shouldVisit(_ kind: SyntaxKind) -> Bool { | ||
| return true | ||
| } | ||
| open func shouldVisit(_ kind: TokenKind) -> Bool { |
There was a problem hiding this comment.
Nitpick: Missing empty line between functions.
| /// not interesting to users' SyntaxVisitor. | ||
| class RawSyntaxVisitor { | ||
| private let visitor: SyntaxVisitor | ||
| private var currentNode: PendingSyntaxNode? |
There was a problem hiding this comment.
Wouldn‘t it be cleaner to habe currentNode be non-optional and make moveUp a no-op when we are already at the root? Or make this an IUO, because we always force unwrap it anyway and I can’t think of any reason where checking it for nil would make sense.
Migrate leading trivia when adding an access-level modifier.
This implementation allows users to specify interested SyntaxNodeKind
and TokenKind; thus we can traverse the RawSyntax Tree instead of
SyntaxTree and realize only the interested RawSyntax nodes into
SyntaxNodes.