Simplify consumption of prefixes in a token #1910
Conversation
|
@swift-ci Please test |
| } else { | ||
| (unexpectedBeforeRangle, rangle) = self.expect(.rightAngle) | ||
| } | ||
| let rangle = self.expectWithoutRecovery(prefix: ">", as: .rightAngle) |
There was a problem hiding this comment.
Did you remove the recovery because you found it didn't actually work as expected?
I was expecting us to parse protocol Test<Foo Bar> {} mostly properly, ie. with just Bar as unexpected. But that doesn't happen as the > in recovery is parsed as a postfix operator (not a right angle) and thus what actually happens is that we skip up to the {. Did you decide it wasn't worth handling that case?
There was a problem hiding this comment.
Yes, I couldn’t find any examples where the recovery kicked in.
We certainly could have expect(prefix:as:) but it’s implementation is non-trivial because non of the current recovery infrastructure supports recovery a to somewhere inside a token. I filed #1923 and maybe we can implement it in a follow-up PR.
There was a problem hiding this comment.
Thanks, I wonder if there's other cases where this is happening (ie. no recovery because we're looking for a prefix).
There was a problem hiding this comment.
That’s happening everywhere we currently expect a prefix. It would be nice to fix at some point.
| @@ -23,6 +23,13 @@ protocol TokenConsumer { | |||
| /// Consume the current token and change its token kind to `remappedTokenKind`. | |||
| mutating func consumeAnyToken(remapping remappedTokenKind: RawTokenKind) -> Token | |||
|
|
|||
There was a problem hiding this comment.
Do we ever take a prefix where it's not just the default text of the token? Could we just pass the token in rather than the prefix as well?
There was a problem hiding this comment.
Yes, there’s
and
Good idea though, I added a precondition to check that prefix is tokenKind.defaultText if that tokenKind has a defaultText.
There was a problem hiding this comment.
If they're the minority, could still be worth having a consumePrefix that only takes the token kind instead. Maybe this is more clear as to what's happening though 🤷
There was a problem hiding this comment.
The problem is that this method doesn’t have a clearly defined meaning if defaultText == nil. I prefer being explicit here and passing the prefix.
ahoppen
force-pushed
the
ahoppen/prefix-consumption
branch
from
7fd6414 to
4527338
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
In essence, introduce
at(prefix:)andconsume(ifPrefix:)and use them in the codebase.I measured performance and there’s no performance regression associated with this change (based on parsing MovieSwiftUI).