Skip to content
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

Borrowed types are Copy so their methods should take self #5921

Open
sffc opened this issue Dec 18, 2024 · 6 comments
Open

Borrowed types are Copy so their methods should take self #5921

sffc opened this issue Dec 18, 2024 · 6 comments
Assignees
Labels
C-meta Component: Relating to ICU4X as a whole S-small Size: One afternoon (small bug fix or enhancement)

Comments

@sffc
Copy link
Member

sffc commented Dec 18, 2024

For example, TimeZoneIdMapperWithFastCanonicalizationBorrowed.

Is there a Clippy warning for this? @Manishearth

@sffc sffc added the C-meta Component: Relating to ICU4X as a whole label Dec 18, 2024
@sffc sffc added this to the ICU4X 2.0 Stretch ⟨P2⟩ milestone Dec 18, 2024
@Manishearth
Copy link
Member

Can't find a clippy warning.

@sffc
Copy link
Member Author

sffc commented Dec 19, 2024

If there's no clippy warning, then I think we have no real choice except to make this a best-effort kind of thing.

@sffc
Copy link
Member Author

sffc commented Dec 19, 2024

@Manishearth
Copy link
Member

Manishearth commented Dec 19, 2024

We should just enable trivially_copy_pass_by_ref. It has a decent threshold set: it warns for types that are of the size usize * 2, which is roughly the threshold where pass by value matters.

We'd need to enable the avoid-breaking-exported-api setting to be allowed to use this

@Manishearth Manishearth self-assigned this Jan 7, 2025
@Manishearth Manishearth added the S-small Size: One afternoon (small bug fix or enhancement) label Jan 7, 2025
@Manishearth
Copy link
Member

2.0: Manish to audit

@sffc
Copy link
Member Author

sffc commented Jan 7, 2025

In general, audit that things that are usize * 2 are using copy self, and likewise that things bigger than usize * 2 should maybe be using &self. (both directions)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-meta Component: Relating to ICU4X as a whole S-small Size: One afternoon (small bug fix or enhancement)
Projects
None yet
Development

No branches or pull requests

2 participants