[NFC] Rename 'SyntaxArena' to 'RawSyntaxArena' #2926
Conversation
|
@swift-ci Please test |
1 similar comment
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Just a few comments regarding naming / comments, otherwise LGTM.
| /// conformance. | ||
| @_spi(RawSyntax) | ||
| public struct SyntaxArenaAllocatedPointer<Element: Sendable>: @unchecked Sendable { | ||
| public struct ArenaAllocatedPointer<Element: Sendable>: @unchecked Sendable { |
There was a problem hiding this comment.
Should the comment also be updated so it doesn’t mention RawSyntaxArena if this type is supposed to hold pointers allocated in arbitrary arenas?
Same for comments below.
Sources/SwiftSyntax/Syntax.swift
Outdated
| // For root node. | ||
| struct Root: Sendable { | ||
| private var arena: RetainedSyntaxArena | ||
| private var rawNodeArena: RetainedRawSyntaxArena |
There was a problem hiding this comment.
You didn’t change all parameters / members from arena to rawNodeArena (eg. RawSyntaxTokenView still has arena: RawSyntaxArena). I think it would be good to have a consistent naming and I don’t see a problem with continuing to call it arena.
Same in some more places.
There was a problem hiding this comment.
I missed some places, but my intention was to leave arena: for RawSyntax operations. But for Syntax related operations, I thought it'd be clearer to use rawAllocationArena or rawNodeArena. Especially if we'd merge #2925 (let's discuss about it next year), Syntax has different "arena".
| /// be reference-counted, further improving the performance of ``SwiftSyntax`` | ||
| /// when worked with at that level. | ||
| public class SyntaxArena { | ||
| public class RawSyntaxArena { |
There was a problem hiding this comment.
It's unfortunate that SyntaxArena was non-SPI public. I'm going to add deprecated typealias SyntaxArena = RawSyntaxArena. Same for ParsingSyntaxArena
There was a problem hiding this comment.
Alex and I discussed about this and we think they shouldn't be public. Making them SPI in #2930 before this PR.
rintaro
force-pushed
the
raw-syntax-arena
branch
2 times, most recently
from
38c9f80 to
39ec125
Compare
|
@swift-ci Please test |
rintaro
force-pushed
the
raw-syntax-arena
branch
from
39ec125 to
78053fb
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
rintaro
force-pushed
the
raw-syntax-arena
branch
from
78053fb to
69d2897
Compare
|
@swift-ci Please test |
The current `SyntaxArena` only manages `RawSyntax` green tree things. In swift-syntax, "Syntax" refers to `Syntax` the red tree. So the naming was a bit confusing. Also rename `SyntaxArenaAllocated(Buffer)Pointer` to 'ArenaAllocated*' in preparation for future introduction of other "arenas".
rintaro
force-pushed
the
raw-syntax-arena
branch
from
69d2897 to
d9882f0
Compare
|
@swift-ci Please test |
2 participants
The current
SyntaxArenaonly managesRawSyntaxgreen tree things. In swift-syntax, "Syntax" refers toSyntaxthe red tree. So the naming was a bit confusing.As for
arena:argument labels, rename it torawAllocationArena:expect in theRawSyntaxworld (i.e.SwiftSyntax/raw/*andSwiftParser/*).Also rename
SyntaxArenaAllocated(Buffer)PointertoArenaAllocated*in preparation for future introduction of other "arenas". (i.e. #2925)