-
Notifications
You must be signed in to change notification settings - Fork 13.5k
completely deduplicate Visitor
and MutVisitor
#142706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
just using `walk_item` instead would be okay.
68ac46d
to
e59533d
Compare
e59533d
to
ed397ad
Compare
@bors2 try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
completely deduplicate `Visitor` and `MutVisitor` r? oli-obk This closes #127615. ### Discussion > * Give every `MutVisitor::visit_*` method a corresponding `flat_map_*` method. Not every AST node exists in a location where they can be mapped to multiple instances of themselves. Not every AST node exists in a location where they can be removed from existence (e.g. `filter_map_expr`). I don't think this is doable. > * Give every `MutVisitor::visit_*` method a corresponding `Visitor` method and vice versa The only three remaining method-level asymmetries after this PR are `visit_stmt` and `visit_nested_use_tree` (only on `Visitor`) and `visit_span` (only on `MutVisitor`). `visit_stmt` doesn't seem applicable to `MutVisitor` because `walk_flat_map_stmt_kind` will ask `flat_map_item` / `filter_map_expr` to potentially turn a single `Stmt` to multiple based on what a visitor wants. So only using `flat_map_stmt` seems appropriate. `visit_nested_use_tree` is used for `rustc_resolve` to track stuff. Not useful for `MutVisitor` for now. `visit_span` is currently not used for `MutVisitor` already, it was just kept in case we want to revive #127241. cc `@cjgillot` maybe we could remove for now and re-insert later if we find a use-case? It does involve some extra effort to maintain. * Remaining FIXMEs `visit_lifetime` has an extra param for `Visitor` that's not in `MutVisitor`. This is again something only used by `rustc_resolve`. I think we can keep that symmetry for now.
This comment has been minimized.
This comment has been minimized.
ed397ad
to
3da58e6
Compare
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (a1b90e8): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 0.9%, secondary -3.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 692.997s -> 691.913s (-0.16%) |
yea that makes sense. The scheme you have now that generates the flat map functions and similar resolves that point sufficiently imo @bors r+ |
r? oli-obk
This closes #127615.
Discussion
Not every AST node exists in a location where they can be mapped to multiple instances of themselves. Not every AST node exists in a location where they can be removed from existence (e.g.
filter_map_expr
). I don't think this is doable.The only three remaining method-level asymmetries after this PR are
visit_stmt
andvisit_nested_use_tree
(only onVisitor
) andvisit_span
(only onMutVisitor
).visit_stmt
doesn't seem applicable toMutVisitor
becausewalk_flat_map_stmt_kind
will askflat_map_item
/filter_map_expr
to potentially turn a singleStmt
to multiple based on what a visitor wants. So only usingflat_map_stmt
seems appropriate.visit_nested_use_tree
is used forrustc_resolve
to track stuff. Not useful forMutVisitor
for now.visit_span
is currently not used forMutVisitor
already, it was just kept in case we want to revive #127241. cc @cjgillot maybe we could remove for now and re-insert later if we find a use-case? It does involve some extra effort to maintain.visit_lifetime
has an extra param forVisitor
that's not inMutVisitor
. This is again something only used byrustc_resolve
. I think we can keep that symmetry for now.