fix: BlockDecomposer #2128
fix: BlockDecomposer #2128
Conversation
tmontaigu
force-pushed
the
tm/blkdcmp
branch
4 times, most recently
from
2125b11 to
2ffefe7
Compare
| fn new_(value: T, bits_per_block: u32, limit: Option<T>, padding_bit: T) -> Self { | ||
| fn new_(value: T, bits_per_block: u32, limit: Option<T>, padding_bit: Option<u32>) -> Self { | ||
| if let Some(padding_bit) = padding_bit { | ||
| assert!(padding_bit <= 1); |
There was a problem hiding this comment.
how about a PaddingBit struct which holds an Option<?> with repr(transparent) and a new_one(), new_zero() function ?
? can be bool or u32 as you like
will avoid the assert to enforce the invariant, wdyt ?
you could even have consts like PADDING_BIT_ZERO, PADDING_BIT_ONE if you like
There was a problem hiding this comment.
I think bool would be enough
There was a problem hiding this comment.
I mean, would it be ok if only the padding_bit argument of the constructors is a bool ?
There was a problem hiding this comment.
ah, don't you need the Option thing to discrimnate some specific case ?
There was a problem hiding this comment.
Its only needed in the struct itself and in the private (new_) constructor as we use a different constructor if the user whishes to specify the padding
There was a problem hiding this comment.
I just don't know if the bool is the best tool to express what the user wants, don't recall if this struct is pub or pub(crate)
There was a problem hiding this comment.
Then I don't feel like bool is the best to carry intent here
There was a problem hiding this comment.
So you'd prefer a enum PaddingBitValue { Zero, One } and possibly None ?
The BlockDecomposer gave the possibility when the number of bits per block was not a multiple of the number of bits in the original integer to force the extra bits of the last block to a particular value. However, the way this was done could only work when setting these bits to 1, when wanting to set them to 0 it would not work. Good news is that we actually never wanted to set them to 0, but it should still be fixed for completeness, and allow other feature to be added without bugs
The BlockDecomposer gave the possibility when the number of bits per block was not a multiple of the number of bits in the original integer to force the extra bits of the last block to a particular value.
However, the way this was done could only work when setting these bits to 1, when wanting to set them to 0 it would not work.
Good news is that we actually never wanted to set them to 0, but it should still be fixed for completeness, and allow other feature to be added without bugs