Handle incorrect VZV bytes representation #3883
Conversation
|
IMO empty VZVs should canonically be the empty slice |
robertbastian
changed the title
| @@ -451,8 +455,14 @@ where | |||
| { | |||
| #[inline] | |||
| fn eq(&self, other: &VarZeroVec<'b, T, F>) -> bool { | |||
| // VarULE has an API guarantee that this is equivalent | |||
| // to `T::VarULE::eq()` | |||
| // VZV::from_elements used to produce a non-canonical representation of the | |||
There was a problem hiding this comment.
I'm fine allowing this to slip
There was a problem hiding this comment.
though this is ok too, note that this also breaks the VarULE "equality is byte equality" invariant on VZS
There was a problem hiding this comment.
if we remove this we should also special case is_empty to check for empty length
There was a problem hiding this comment.
is_empty should already work, because it checks VZComponent's indices slice, which gets special-case initialised.
note that this also breaks the VarULE "equality is byte equality" invariant on VZS
We already broke that invariant
There was a problem hiding this comment.
Note that testdata CI is failing so the incorrect representation is out there
There was a problem hiding this comment.
Alright, that's fine by me then, since it's legacy-only. We may wish to deprecate this in the long run.
|
I slightly lean toward |
The empty VZV is currently either
[], when created withVZV::new/VZS::new_empty, or[0,0,0,0]when created withVZV::from(&[]). This breaks the byte equality invariant thatPartialEq, among others, relies on.This fix makesVZS::new_emptyreturn[0,0,0,0]. However, as there is data with the[]out in the wild, we probably have to adapt at leastPartialEqto handle it.This fix make
VZV::fromreturn[]for empty input, and updates thePartialEqimplementation to deal with the non-canonical representation.