change usize to u64 in many places that represent values not indexes and reduce error surface #106
Conversation
There was a problem hiding this comment.
Thank you very much for your many contributions!
This PR should eliminate many redundant runtime errors and significantly improve code stability. I've added comments. Could you address them?
A quick note: Please don't remove the ? operators from the Rustdoc Examples. These examples also serve as unit tests, and it's important that errors are detected.
src/mii_sequences/elias_fano.rs
Outdated
| let e = EliasFano::from_bits([]); | ||
| assert_eq!( | ||
| e.err().map(|x| x.to_string()), | ||
| Some("bits must not be empty.".to_string()) | ||
| ); |
There was a problem hiding this comment.
The assert_eq should not be omitted because it is expected that the error value is returned.
There was a problem hiding this comment.
Sorry for making extra work for you, this was again a point where I thought the error would not occur anymore.
| /// | ||
| /// An error is returned if `num_vals == 0`. | ||
| pub fn new(universe: usize, num_vals: usize) -> Result<Self> { | ||
| pub fn new(universe: u64, num_vals: usize) -> Self { |
There was a problem hiding this comment.
Could you not remove this error handling? EliasFano's behavior is undefined when given empty input. For example, an EliasFano instance created by EliasFanoBuilder::build() with num_vals==0 cannot be guaranteed to function correctly.
There was a problem hiding this comment.
OK I will revert this! I thought it is like a vector or array which can be empty.
| assert_eq!( | ||
| e.err().map(|x| x.to_string()), | ||
| Some("num_vals must not be zero.".to_string()) | ||
| ); |
There was a problem hiding this comment.
Could you not remove the assertion? The returned value should be evaluated.
| /// let mut efb = EliasFanoBuilder::new(8, 4); | ||
| /// efb.extend([1, 3, 3, 7]); |
| /// let mut efb = EliasFanoBuilder::new(8, 4); | ||
| /// efb.extend([1, 3, 3, 7]); |
| /// let mut efb = EliasFanoBuilder::new(8, 4); | ||
| /// efb.extend([1, 3, 3, 7]); |
| /// let mut efb = EliasFanoBuilder::new(8, 4); | ||
| /// efb.extend([1, 3, 3, 7]); |
| /// let mut efb = EliasFanoBuilder::new(8, 4); | ||
| /// efb.extend([1, 3, 3, 7]); |
Co-authored-by: Shunsuke Kanda <shnsk.knd@gmail.com>
Co-authored-by: Shunsuke Kanda <shnsk.knd@gmail.com>
|
@kampersanda I'm working on reverting the reverting back the cases where I went too far with the error handling simplification but there is now a problem with Elias-Fano: Because the other data structures cannot cause runtime anymore I changed the trait to return the structure directly, but this conflicts with Elias-Fano where an error can still occur if the slice is empty, which is why I tried to make it behave like other data structures where them being empty is a valid state.
|
The second option looks good. It could be achieved by changing pub struct PrefixSummedEliasFano {
- ef: EliasFano,
+ ef: Option<EliasFano>,
}That is, if the input is empty, |
As proposed at #104, this mainly makes it easier to extend and create bit vectors with less error handling on run time and more at compile time, see the issue for more details.
This had huge ripple effects and took a lot of time so by the end I was rushing it a bit, I mean I got all tests and doc tests to compile and run successfully but still I think you need to check this in case I overlooked something, there is also surely more to do in the future in this area.
As usize is the same as u64 for the target bit width of 64 bit this is all a bit strange so I tried to make a quick call as to what the intent is (index vs value).
However in this library it is sometimes mixed or used to calculate one from the other, so I was not sure in all cases but also looked up how another library or the intrinsics are defined.
Please merge into a single commit, the commits of the PR before the last one do not compile or test correctly.
By the way I just saw at Cydhra/vers#34 that similar libraries seem to plan a similar refactoring at the same time :-)