Fix MultiFieldsULE #3642
Fix MultiFieldsULE #3642
Conversation
| @@ -44,7 +44,7 @@ impl MultiFieldsULE { | |||
| lengths, output, | |||
| ); | |||
| debug_assert!( | |||
| <VarZeroSlice<[u8]>>::validate_byte_slice(output).is_ok(), | |||
| <VarZeroSlice<[u8], Index32>>::validate_byte_slice(output).is_ok(), | |||
There was a problem hiding this comment.
Good sleuthing!
However, I think we should prefer Index16. No reason to use the extra bytes.
There was a problem hiding this comment.
Yeah, this is intended to default to Index16. In the long run we can introduce a derive config flag for this.
Also it's a data breaking change to change the format here, though I don't think ICU4X uses this yet (it will once casemap stabilizes)
There was a problem hiding this comment.
wait, no, nvm, it should default to 32 since we expect the lengths to be large: this may be at the root of something sufficiently nested
for users who want 16 we should add a default type parameter and a zerovec attribute
There was a problem hiding this comment.
That's the issue of this PR, it is Index32 but not consistently. Switching to Index16 would be a breaking change as I understand
There was a problem hiding this comment.
(VarZeroVec defaults to Index16, but MFULE overrides that to Index32)
MultiFieldsULE uses
Index32, but this wasn't passed explicitly everywhere, so some places incorrectly defaulted toIndex16.The existing test suite did not discover this error, but I'm not sure why. Added a new struct with four internal VZVs, which triggers the bug. A struct with three internal VZVs also does not trigger the bug.