parser: Collapse parser and tokenizer. - #456
Conversation
Now they are 1:1 so the distinction is mostly internal. This allows state tracking to be a bit simpler.
|
@SimonSapin not sure how you feel about this, the main point I wanted to get out of this is to be able to not track at_start_of separately, but I think it makes sense in general now that |
|
Aren’t there tests that run the tokenizer by itsef? |
c4ff03b to
d520608
Compare
Not really |
|
Fixed CI (msrv didn't support the |
emilio
left a comment
emilio
left a comment
There was a problem hiding this comment.
@SimonSapin does this look reasonable to you? The comment below is the only change that I think is potentially dubious / objectionable in practice, but I'd rather do either nothing (this patch), or the look_for_arbitrary_substitution_functions bit suggested below, than the existing thing which penalizes all tokens all the time.
| self.tokenizer.reset(&cached_token.end_state); | ||
| if let Token::Function(ref name) = cached_token.token { | ||
| self.tokenizer.see_function(name) | ||
| } |
There was a problem hiding this comment.
Note that this removal is an intentional change, I want to make the cached token path add as little overhead as possible. This required a change in servo when parsing declaration blocks but I think this is ok. The alternative would be to make look_for_arbitrary_substitution_functions invalidate the cached token, at least if it's a function (but we'd need the same change anyway to avoid the performance regression).
|
Tests would be nice, which would also show how using the |
Tests for what? The public API doesn't change at all. |
|
FWIW I have a try run with this patch green on Firefox which include all of WPT and mochitests, etc so not too concerned about introducing regressions here. But happy to add coverage for any particular area that you think might deserve it? But seems tangential / worth a separate PR in any case. |
tiaanl
left a comment
tiaanl
left a comment
There was a problem hiding this comment.
With the caching change handled elsewhere, I don't see issues with it.
|
It’s been years since I’ve properly looked at cssparser code so this isn’t a full review (sorry!) but I’m not opposed to this in principle. Especially having already ported Firefox to this change with green test sounds good. Since that includes Stylo, I assume Servo should be mostly fine too. |
|
Yeah pretty sure this change is a no-op except for that very specific |
3 participants
Now they are 1:1 so the distinction is mostly internal. This allows
state tracking to be a bit simpler.