Skip to content

Make trivial-copy-size-limit consistently the size of the target pointer #13319

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

Merged
merged 1 commit into from
May 21, 2025

Conversation

Alexendoo
Copy link
Member

@Alexendoo Alexendoo commented Aug 28, 2024

Fixes https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Ambiguous.20default.20value.20for.20.60trivial-copy-size-limit.60

The current situation is

Target width trivial-copy-size-limit
8-bit 2
16-bit 4
32-bit 8
64-bit 8

Since practically speaking it's almost always 8, let's go with that as the unconditional default to make it easier to understand

Now defaults to target_pointer_width

changelog: [trivial-copy-size-limit] now also defaults to the size of a target pointer (unchanged for 64-bit targets)

@rustbot
Copy link
Collaborator

rustbot commented Aug 28, 2024

r? @Jarcho

rustbot has assigned @Jarcho.
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 Aug 28, 2024
@rust-cloud-vms rust-cloud-vms bot force-pushed the trivial-copy-size-limit branch from 0d48fc4 to e966c5e Compare August 28, 2024 18:06
@Jarcho
Copy link
Contributor

Jarcho commented Aug 28, 2024

The original justification for limiting it to eight on 64-bit targets was quite flimsy. It makes (usize, usize) ok on everything but 64-bit targets. This means something like struct S(&'static [u8]) is too big to pass by value, which is rather ridiculous.

It would be nice if the lint could be target independent, but struct sizes already aren't.

@Alexendoo
Copy link
Member Author

This means something like struct S(&'static [u8]) is too big to pass by value

It means it's not small enough to lint, not that it's too big to pass by value. That's a separate config pass-by-value-size-limit

@Jarcho
Copy link
Contributor

Jarcho commented Aug 29, 2024

Sorry I had lint/not lint backwards (was half thinking of large_types_passed_by_value). &S would be trigger trivially_copy_pass_by_ref on 32-bit targets, but not on 64-bit targets. Basically fat pointers should behave consistently.

@bors
Copy link
Contributor

bors commented Oct 1, 2024

☔ The latest upstream changes (presumably #13286) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot

This comment has been minimized.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 31, 2025
@Alexendoo Alexendoo force-pushed the trivial-copy-size-limit branch from e966c5e to 849ed1a Compare April 2, 2025 14:30
@Alexendoo Alexendoo changed the title Make trivial-copy-size-limit target independent Make trivial-copy-size-limit consistently the size of the target pointer Apr 2, 2025
@Alexendoo
Copy link
Member Author

Changed it to be the width of the target pointer, this halves the limit for smaller targets but keeps 64-bit the same

Increasing 64-bit's limit is also an option, but it led to many more cases being linted

@Alexendoo Alexendoo added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 2, 2025
@Alexendoo Alexendoo force-pushed the trivial-copy-size-limit branch from 849ed1a to ae0e3b7 Compare April 3, 2025 12:18
@Jarcho Jarcho added this pull request to the merge queue May 21, 2025
Merged via the queue into rust-lang:master with commit 9fa448a May 21, 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.

5 participants