parser: Remove ParserInput. - #446
Conversation
This avoids unnecessary indirection during parsing. We only create nested parsers in two places, and it seems easy to avoid.
|
@SimonSapin you have more historical context than me, can you think of any reason not to do this? |
|
This has been reviewed upstream in https://phabricator.services.mozilla.com/D321882 but will wait a bit before merging (at least a day or two) to get performance numbers (though I think we should do this just for simplicity), and also to wait for Simon's or other folk's feedback :) |
| if let Some(block_type) = delimited_parser.at_start_of { | ||
| consume_until_end_of_block(block_type, &mut delimited_parser.input.tokenizer); | ||
| } | ||
| parser.stop_before = delimiters; |
There was a problem hiding this comment.
This is the main tricky change really.
| let result; | ||
| // Introduce a new scope to limit duration of nested_parser’s borrow | ||
| { | ||
| let mut nested_parser = Parser { |
|
IIRC there's some historical context around this in the git log (on my phone, so can't look it up right now). Would be nice if it can be removed. I think it makes the API a little less flexible around input? Aside from this change, some docs on how to construct and use Parser (and ParserInput if it stays) would be really nice. The lib.rs docs show how to implement a Parser, but not really how to use one. |
|
Err, yeah so... So I think this is fine nowadays. Why do you think this makes the API less flexible? I don't see how. |
|
I feel there must have been a reason to bother with two lifetime parameters but right now I can’t remember what it is or was. Stylo is probably the biggest consumer of cssparser, so if you’ve ported it to this new API and feel it’s an improvement I trust your judgment Emilio. |
Possibly because the code was written before the stabilization of non-lexical lifetimes in the 2018 edition. |
4 participants
This avoids unnecessary indirection during parsing. We only create nested parsers in two places, and it seems easy to avoid.