Skip to content

Use ControlFlow in more visitors #12829

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.

Sign up for GitHub

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

Jump to bottom
Closed
y21 opened this issue May 21, 2024 · 1 comment · Fixed by #12830
Closed

Use ControlFlow in more visitors #12829

y21 opened this issue May 21, 2024 · 1 comment · Fixed by #12830
Assignees
@blyxyas
Labels
performance-project For issues and PRs related to the Clippy Performance Project

Comments

@y21
Copy link
Member

y21 commented May 21, 2024 •
edited by rustbot
Loading

Description

As of "recently" the Visitor and TypeVisitor traits have an optional associated Result type that can be used to early exit the visitor, by setting type Result = ControlFlow<()> (instead of the implicit type Result = ()).

A lot of lints (and utils) use visitors to search for something in HIR nodes and try to avoid doing a bunch of extra work by storing something like a bool for "done" and then ignore any visit_* calls when set to true, but rustc will still keep calling into the visitor (for e.g. the remaining statements in a block), so it seems like a good idea to make use of this associated type by setting type Result = ControlFlow<()>; in more places.

This seems especially useful for e.g. for_each_expr, where a lot of lints already do return ControlFlow.

Version

No response

Additional Labels

No response
@blyxyas
Copy link
Member

blyxyas commented May 21, 2024

@rustbot claim

@blyxyas blyxyas added the performance-project For issues and PRs related to the Clippy Performance Project label May 21, 2024
@blyxyas blyxyas mentioned this issue May 21, 2024
Merged
bors added a commit that referenced this issue Jul 24, 2024
@bors
Auto merge of #12830 - blyxyas:more-controlflow, r=y21,xFrednet
Loading
Loading status checks…
fcecf3f 
Use ControlFlow in more places

Now, instead of manually using variables in visitors to signify that a visit is "done" and that the visitor should stop traversing. We use the trait type "Result" to signify this (in relevant places).

I'll schedule a perf run, I don't think it will be much of a difference, but every bit of performance is welcomed :)

Fixes #12829
r? `@y21`
@bors bors closed this as completed in 16953ce Jul 24, 2024
@blyxyas blyxyas mentioned this issue Jul 24, 2024
Open
2 tasks
@y21 y21 mentioned this issue Jul 25, 2024
Merged
bors added a commit that referenced this issue Jul 25, 2024
@bors
Auto merge of #13156 - y21:for_each_expr_visitor_refactor, r=xFrednet
Loading
Loading status checks…
c7cfe7f 
Remove unnecessary `res` field in `for_each_expr` visitors

Small refactor in the `for_each_expr*` visitors. This should not change anything functionally.

Instead of storing the final value `Option<B>` in the visitor and setting it to `Some` when we get a `ControlFlow::Break(B)` from the closure, we can just directly return it from the visitor itself now that visitors support that.

cc #12829 and #12830 (comment)

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@blyxyas blyxyas

Labels
performance-project For issues and PRs related to the Clippy Performance Project
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use ControlFlow in more places blyxyas/rust-clippy
2 participants
@y21 @blyxyas