[libSyntax] Create SyntaxRef, which uses SyntaxDataRef internally #36350
Conversation
|
@swift-ci Please smoke test |
For syntax nodes that previously didn’t have a `validate` method, the newly added `validate` method is a no-op. This will make validation easier in upcoming generic code.
|
@swift-ci Please smoke test |
include/swift/Syntax/Syntax.h
Outdated
|
|
||
| virtual ~Syntax() {} | ||
| explicit Syntax(const RC<const SyntaxData> &Data) : Data(Data) { | ||
| assert(Data != nullptr && "SyntaxData must be backed by Data"); |
There was a problem hiding this comment.
You mean "Syntax must be backed by Data"?
| class Syntax { | ||
| protected: | ||
| RC<const SyntaxData> Data; | ||
|
|
||
| public: | ||
| explicit Syntax(const RC<const SyntaxData> &Data) : Data(Data) {} | ||
|
|
||
| virtual ~Syntax() {} |
There was a problem hiding this comment.
Do you know why this was there? If not, never mind. If yes, why removing this?
There was a problem hiding this comment.
I guess that the specific Syntax nodes had their own destructors at some point and that they went away during some change (none of my recent ones anyway). I removed it, nothing broke, so I assumed it’s not necessary and removed it just to clear things up. There’s no specific reason to keep or remove it.
include/swift/Syntax/Syntax.h
Outdated
|
|
||
| /// See comment on top of file. | ||
| class SyntaxRef { | ||
| const SyntaxDataRef *Data; |
There was a problem hiding this comment.
I found the name SyntaxDataRef is confusing. It sounds like "reference to SyntaxData", but it is actually the data itself. But I couldn't come up with a better name..
There was a problem hiding this comment.
As discussed offline, I’ll set up a follow-up PR to change the name of SyntaxDataRef. On there, we can also discuss what a better name might be.
|
@swift-ci Please smoke test |
include/swift/Syntax/Syntax.h
Outdated
|
|
||
| /// See comment on top of file. | ||
| class SyntaxRef { | ||
| const SyntaxDataRef *Data; |
There was a problem hiding this comment.
I think the pointer itself is also never changed. So const SyntaxDataRef * const Data
There was a problem hiding this comment.
👍 Also changed Data in Syntax to const RC<const SyntaxData> Data.
| assert(Ref.getDataRef() == getDataPtr() && | ||
| "Ref no longer pointing to Data?"); |
There was a problem hiding this comment.
If Data is const, this assert is not needed.
There was a problem hiding this comment.
I ran into issues here at some point after moving OwnedSyntaxRef to a different location. The pointer stays constant but the SyntaxDataRef now lives in a different location, so the pointer isn’t valid anymore. That’s why I put the assertion in and I think it’s still useful if someone starts to fiddle around OwnedSyntaxRef.
Now that we have a fast SyntaxDataRef, create a corresponding SyntaxRef hierarchy. In contrast to the Syntax, SyntaxRef does *not* own the backing SyntaxDataRef, but merely has a pointer to the SyntaxDataRef. In addition to the requirements imposed by SyntaxDataRef, the user of the SyntaxRef hierarchy needs to make sure that the backing SyntaxDataRef of a SyntaxRef node stays alive. While this sounds like a lot of requirements, it has performance advantages: - Passing a SyntaxRef node around is just passing a pointer around. - When casting a SyntaxRef node, we only need to create a new SyntaxRef (aka. pointer) that points to the same underlying SyntaxDataRef - there's no need to duplicate the SyntaxDataRef. - As SyntaxDataRef is not ref-counted, there's no ref-counting overhead involved. Furthermore, the requirements are typically fulfilled. The getChild methods on SyntaxRef return an OwnedSyntaxRef, which stores the SyntaxDataRef. As long as this variable is stored somewhere on the stack, the corresponding SyntaxRef can safely be used for the duration of the stack frame. Even calls like the following are possible, because OwnedSyntaxRef returned by getChild stays alive for the duration of the entire statement. ``` useSyntaxRef(mySyntaxRef.getChild(0).getRef()) ```
|
@swift-ci Please smoke test |
Now that we have a fast
SyntaxDataRef, create a correspondingSyntaxRefhierarchy. In contrast to theSyntaxhierarchy,SyntaxRefdoes not own the backingSyntaxDataRef, but merely has a pointer to theSyntaxDataRef. In addition to the requirements imposed bySyntaxDataRef, the user of theSyntaxRefhierarchy needs to make sure that the backingSyntaxDataRefof aSyntaxRefnode stays alive. While this sounds like a lot of requirements, it has performance advantages:SyntaxRefnode around is just passing a pointer around.SyntaxRefnode, we only need to create a newSyntaxRef(aka. pointer) that points to the same underlyingSyntaxDataRef– there's no need to duplicate theSyntaxDataRef.SyntaxDataRefis not ref-counted, there's no ref-counting overhead involved.Furthermore, the requirements are typically fulfilled. The
getChildmethods on SyntaxRef return anOwnedSyntaxRef, which stores theSyntaxDataRef. As long as this variable is stored somewhere on the stack, the correspondingSyntaxRefcan safely be used for the duration of the stack frame. Even calls like the following are possible, becauseOwnedSyntaxRefreturned bygetChildstays alive for the duration of the entire statement.