Make ULE types Copy, add PairULE #1193
Conversation
Manishearth
requested review from
gregtatum,
nordzilla,
sffc,
zbraniecki and
a team
as code owners
|
This does minor refactorings of a lot of code, which is why it tagged everyone in as an owner. I think after this I'm going to cut a version of ZeroVec so that tinystr can be brought up to date, we've had a lot of changes and I'd like them to be published. I still have stuff to do but this is a reasonable milestone since almost all of the other stuff is additive. |
| /// We do not have guarantees for the layouts of tuples, so we must define a custom | ||
| /// ULE type for pairs. This could potentially be generalized for larger tuples if necessary | ||
| #[repr(packed)] | ||
| pub struct PairULE<A, B>(pub A, pub B); |
There was a problem hiding this comment.
Suggestion: can we make a 3-part and 4-part ULE as well? I think we may need at least 3 for the LocaleCanonicalizer stuff (language script region).
There was a problem hiding this comment.
My understanding was that those were maps from language to script/region? We can add the 3-part one when needed imo, it should be easy to copy PairULE
| IncorrectLength(/* expected */ usize, /* found */ usize), | ||
| } | ||
|
|
||
| impl<E: fmt::Display, F: fmt::Display> fmt::Display for PairULEError<E, F> { |
There was a problem hiding this comment.
Suggestion (optional): it would be nicer to use displaydoc but I know you don't want to add that dep
There was a problem hiding this comment.
We can't use derives on this type
Fixes #1184
I was adding PairULE and was hitting rust-lang/rust#82523 (comment) . I decided to use this opportunity to make things Copy anyway.
I much prefer the new API, there's no need to take superfluous
&references to satisfy the compiler.