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.

Sign up for GitHub

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

Jump to bottom

Consider requiring all ULEs to be Copy #1184

Closed
2 tasks done
sffc opened this issue Oct 18, 2021 · 4 comments · Fixed by #1193
Closed
2 tasks done

Consider requiring all ULEs to be Copy #1184

sffc opened this issue Oct 18, 2021 · 4 comments · Fixed by #1193
Labels
C-data-infra Component: provider, datagen, fallback, adapters S-tiny Size: Less than an hour (trivial fixes) T-techdebt Type: ICU4X code health and tech debt
Milestone
ICU4X 0.4

Comments

@sffc
Copy link
Member

sffc commented Oct 18, 2021 •
edited by Manishearth
Loading

Part of #1082

We currently have the Copy bound on the impl ZeroVec. However, it may make more sense to move it to the trait ULE itself. I know there is a debate about when to put bounds on the impl and when to put them on the trait.

Motivating case: #[derive(Clone)] does not seem to work on structs containing a ZeroVec field. See e.g. #1161

Needs approval from:
@sffc sffc added C-data-infra Component: provider, datagen, fallback, adapters T-techdebt Type: ICU4X code health and tech debt S-tiny Size: Less than an hour (trivial fixes) needs-approval One or more stakeholders need to approve proposal labels Oct 18, 2021
@sffc sffc mentioned this issue Oct 18, 2021
Merged
@Manishearth
Copy link
Member

Yeah that works for me.

@Manishearth
Copy link
Member

The debate is usually for when T itself is being used, when an associated type is used then people need to bound on a trait anyway. ZeroVec already requires ULE on the definition (it has to!) so adding a bound to ULE seems fine

@Manishearth
Copy link
Member

Conceptually I think that AsULEs can also be Copy, and we can stop using references in their methods

@iainireland
Copy link
Contributor

Sounds good to me

This was referenced Oct 20, 2021
Merged
Closed
@Manishearth Manishearth added this to the ICU4X 0.4 milestone Oct 20, 2021
@Manishearth Manishearth mentioned this issue Nov 4, 2021
Closed
@sffc sffc removed the needs-approval One or more stakeholders need to approve proposal label Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-data-infra Component: provider, datagen, fallback, adapters S-tiny Size: Less than an hour (trivial fixes) T-techdebt Type: ICU4X code health and tech debt
Projects
None yet
Milestone
ICU4X 0.4
Development

Successfully merging a pull request may close this issue.

Make ULE types Copy, add PairULE Manishearth/icu4x
3 participants
@sffc @Manishearth @iainireland