-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Optimize needless_bool
lint
#15423
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
Optimize needless_bool
lint
#15423
Conversation
Performance improvements have not been measured experimentally, this is only a reaction to #clippy > Clippy's performance "monthly" update @ 💬. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a naming nit, just verified it in bumpalo
and the lint has gone down from 6.3 million instructions to 179k. That's a 97.1% perf. improvement for the lint.
Great!
clippy_lints/src/needless_bool.rs
Outdated
&& let Some(higher::If { | ||
cond, | ||
then, | ||
r#else: Some(r#else), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be more ergonomic to get rid of this r#else
block and just call it else_block
. I'm not sure why we have so many r#..
identifiers when they're so awkward to use.
r#else: Some(r#else), | |
r#else: Some(else_block), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed into else_expr
, since it might not be a block (only the then
is guaranteed to be one).
Two optimizations have been done when checking for the context in which to apply the lint: - Checking for the mere presence of comments does not require building a `String` with the comment to then check if it is empty and discard it. - Checking for the presence of comment can be done after we have checked that we do have a `if` construct that we intend to lint instead of for every expression.
87a2a7a
to
8602faa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! ❤️ 🌈
Two optimizations have been done when checking for the context in which to apply the lint:
String
with the comment to then check if it is empty and discard it.if
construct that we intend to lint instead of for every expression.changelog: none
r? blyxyas