Adding T: Bake bound on ZeroVec's Bake
#3451
Conversation
robertbastian
requested review from
Manishearth,
sffc,
zbraniecki,
nordzilla,
nciric,
gregtatum,
echeran and
a team
as code owners
robertbastian
requested review from
skius
and removed request for
zbraniecki,
echeran,
gregtatum,
nciric and
nordzilla
| } | ||
| Err(_) => { | ||
| env.insert("zerovec"); | ||
| let u24 = u32::from_le_bytes([self.0[0], self.0[1], self.0[2], 0]); |
There was a problem hiding this comment.
Is there no bake here intentionally?
There was a problem hiding this comment.
Somewhat, it's more code for the same result.
|
Not opposed. Bit worried about how we can manage this for VZV, if at all, but we don't need to. This is a breaking change in databake, though. I think it would be nice if we could mitigate that breakage for one release (let ICU4X's databake dep be a range dep for 1.3). Not blocking, and we can see what it looks like when we do a release. |
|
For VZV we'd probably want a The breaking in databake is in the other PR, no? |
This comment was marked as spam.
This comment was marked as spam.
|
Oh, no, it's breaking |
|
Meh, nobody uses the |
| @@ -8,7 +8,7 @@ use databake::*; | |||
|
|
|||
| impl<T> Bake for ZeroVec<'_, T> | |||
| where | |||
| T: AsULE + ?Sized, | |||
| T: AsULE + ?Sized + Bake, | |||
There was a problem hiding this comment.
This new bound is a breaking change in zerovec. I don't care too much about it though because it only comes up if someone is using both zerovec and databake at the same time, and (most importantly) zerovec is pre-1.0.
There was a problem hiding this comment.
Yes, though we need to figure out if we want to follow semver and release a breaking zerovec bump.
| @@ -15,7 +16,33 @@ impl<const N: usize> Bake for TinyAsciiStr<N> { | |||
| } | |||
| } | |||
|
|
|||
| impl<const N: usize> databake::Bake for UnvalidatedTinyAsciiStr<N> { | |||
There was a problem hiding this comment.
Nit: Please mention these impls in the PR title.
Based on #3450
This bound is currently unused, but will be useful as part of #1935