[Syntax] Don't rebuild Syntax with RawSyntax in SyntaxVisitor #14057
Conversation
We don't guarantee RawSyntax nodes as unique object in the tree, while we do that for SyntaxData. We shouldn't recreate SyntaxData once it was built.
|
@swift-ci Please smoke test |
| } | ||
| } | ||
| return NonTokenChildren; | ||
| return Data->getNumChildren(); |
There was a problem hiding this comment.
I don't see any reason this skips token children.
There was a problem hiding this comment.
I remember writing this and we had some tests that, for some reason, expected it to skip token children. Didn't want to mess with it at the time, because I was doing a behavior-preserving refactor. As far as I'm concerned, it should not skip tokens.
There was a problem hiding this comment.
Originally, we defined children as something indicating recursive structure in the tree but I agree, this should not skip tokens. This may indicate the name children isn't quite right. FWIW, Roslyn likes to use the term slots, similar to how we also use the term layout. Whether you can descend into a node is irrelevant to the number of items in the syntax's layout.
| } | ||
| } | ||
| return NonTokenChildren; | ||
| return Data->getNumChildren(); |
There was a problem hiding this comment.
Originally, we defined children as something indicating recursive structure in the tree but I agree, this should not skip tokens. This may indicate the name children isn't quite right. FWIW, Roslyn likes to use the term slots, similar to how we also use the term layout. Whether you can descend into a node is irrelevant to the number of items in the syntax's layout.
| @@ -181,7 +184,7 @@ class Syntax { | |||
|
|
|||
| /// Get the absolute position of this raw syntax: its offset, line, | |||
| /// and column. | |||
| AbsolutePosition getAbsolutePosition(SourceFileSyntax Root) const; | |||
| AbsolutePosition getAbsolutePosition() const; | |||
|
Thank you for clarifying Ashley and Harlan! |
We don't guarantee
RawSyntaxnodes as unique object in the tree, while we do that forSyntaxData. We shouldn't recreateSyntaxDataonce it's built.RawSyntaxSyntaxDataAlso,
Syntaxnodes have the root node in it sogetAbsolutePosition()doesn't needRootparameter.CC: @nkcsgexi