Skip to content

Conversation

wowinter13
Copy link
Contributor

@wowinter13 wowinter13 commented Jan 15, 2025

resurrection of #10984

fixes #10981

changelog: [sliced_string_as_bytes]: add new lint sliced_string_as_bytes

@rustbot
Copy link
Collaborator

rustbot commented Jan 15, 2025

r? @Manishearth

rustbot has assigned @Manishearth.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 15, 2025
@wowinter13 wowinter13 changed the title WIP (tests to be done): New lint slice_as_bytes New lint slice_as_bytes Jan 15, 2025
/// ```
#[clippy::version = "1.72.0"]
pub SLICE_AS_BYTES,
pedantic,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: performance?

Copy link
Contributor Author

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

Copy link
Member

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?

@wowinter13 wowinter13 changed the title New lint slice_as_bytes New lint sliced_string_as_bytes Jan 15, 2025
@wowinter13 wowinter13 changed the title New lint sliced_string_as_bytes New lint sliced_string_as_bytes Jan 15, 2025
Copy link
Member

@samueltardieu samueltardieu left a 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.

@wowinter13
Copy link
Contributor Author

@Manishearth Would you have a second to check?

@Manishearth
Copy link
Member

I still think it should be a perf lint, unless we're worried that it's going to be super noisy.

Thoughts @xFrednet @flip1995 ? (to get some random additional opinions)

@xFrednet
Copy link
Contributor

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 perf.

Lintcheck shows 4 new lint triggers, so it shouldn't be too noisy.

@Manishearth
Copy link
Member

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.

@wowinter13
Copy link
Contributor Author

@Manishearth changed to perf & resolved conflicts
(It looks like majority stands with perf lint)

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 25, 2025
@rustbot

This comment has been minimized.

@wowinter13 wowinter13 force-pushed the new-lint-slice-as-bytes branch from 9b5b4cb to beeb6f7 Compare January 25, 2025 17:44
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 25, 2025
@Manishearth Manishearth added this pull request to the merge queue Jan 27, 2025
Merged via the queue into rust-lang:master with commit 85bbba6 Jan 27, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flip order of s[a..b].as_bytes()
5 participants