-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
New lint sliced_string_as_bytes
#14002
base: master
Are you sure you want to change the base?
Conversation
r? @Manishearth rustbot has assigned @Manishearth. Use |
slice_as_bytes
slice_as_bytes
clippy_lints/src/methods/mod.rs
Outdated
/// ``` | ||
#[clippy::version = "1.72.0"] | ||
pub SLICE_AS_BYTES, | ||
pedantic, |
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.
nit: performance?
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.
#10984 (comment)
I decided to follow @Jarcho 's category selection
But without a doubt I can revert it back to perf
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.
Yeah, I think the behavior change should not matter. It is indeed a very minor performance win so that's a somewhat compelling argument for pedanticness. Curious what @Jarcho thinks now?
sliced_string_as_bytes
sliced_string_as_bytes
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, with two minor nits.
@Manishearth Would you have a second to check? |
I can't quite make up my mind. It doesn't feel like a warn-by-default lint to me, but I see how it could save performance, especially with long strings. So normally, I'd suggest pedantic as well, but I'd also be okay with your suggestion of Lintcheck shows 4 new lint triggers, so it shouldn't be too noisy. |
I think "minor perf benefit but also not too noisy" makes it a perf lint in my book. I tend to weight things by how likely they are to trigger. |
@Manishearth changed to |
This comment has been minimized.
This comment has been minimized.
9b5b4cb
to
beeb6f7
Compare
resurrection of #10984
fixes #10981
changelog: [
sliced_string_as_bytes
]: add new lintsliced_string_as_bytes