ast: Replace can_parse with static dispatch #10373
Conversation
|
I have no idea why |
|
Known issue, nothing to do with your PR. |
| trait CheckParse { | ||
| /// A true return means we should descend into the production, false means stop. | ||
| fn can_be_parsed(pop: &mut Populator<'_>) -> bool; | ||
| } |
There was a problem hiding this comment.
Really nice work.
Getting rid of virtual dispatch seems like a good direction, a big one would be #9636 (comment)
There was a problem hiding this comment.
I'll look into that once this step is merged.
I'm quite fond of enum_dispatch, but it might not be the best fit here. A custom macro (perhaps with the help of paste) would allow us to deduplicate more code (I'm mostly thinking about the downcasting).
There was a problem hiding this comment.
possibly but we also need to implement generic methods like Node::typ() and Node::source_range(), ideally without splitting the node definition paragraphs.
My original goal was to get rid of the None variants on StatementVariant and BlockStatementHeaderVariant, but I discovered that that would require a larger refactor.
One problem is that those union nodes are not real nodes. It's unclear if they should be but the status quo requires a lot of boilerplate that even spreads to macros (visit_union_field_mut).
Maybe we should remove union nodes like Statement and replace them with something that's not a full node but simply delegates to the concrete statement.
Description
My original goal was to get rid of the
Nonevariants onStatementVariantandBlockStatementHeaderVariant, but I discovered that that would require a larger refactor.This change stands on its own, but doubles as a small step towards that refactor by reducing our reliance on "uninitialized" node values.
No intended behavior changes, but I wouldn't be surprised by a minor performance boost.
TODOs: