Cydhra / vers

Succinct data structures using very efficient rank and select
Apache License 2.0
62 stars 4 forks source link

remove redundant bits_per_element #12

Closed somethingelseentirely closed 2 months ago

somethingelseentirely commented 2 months ago

I figured I'd help out by doing at least some tedious stuff 😅 This is just a replacement of all usages of self.bits_per_element with self.data.len(), and removes the now unnecessary as uints.

Cydhra commented 2 months ago

I'd rather replace self.bits_per_element with self.bits_per_element() which returns self.data.len(). More readable and better if something changes again in how levels are represented (which I am sort of planning to do in a major release)

somethingelseentirely commented 2 months ago

So just use bit_len everywhere?

I think I would make it return an usize then though, because otherwise you'll get a lot of needles truncation operations from the usize -> u16 -> usize roundtrip.

I've come to the conclusion that APIs should in general return usize whenever they are returning something length/index-like, the reduced register pressure is not really worth it compared to the hassle of recasting all the time (I used to have u8 in my code everywhere).

Edit: Or maybe just deprecate bit_len and add the new bits_per_element if you want to keep backwards compatibility.

Edit2: I did the latter 😄

Cydhra commented 2 months ago

Yes this is how I imagined it, and keeping everything usize is probably a good call, I already did that mistake once, when I let count_ones() and count_zeros() in BitVec return u32 to match the equivalent methods on integers, despite this limiting the amount of bits in the vector without any sound reason.

somethingelseentirely commented 2 months ago

Just rebased it onto the master real quick 😄

Cydhra commented 2 months ago

If you open your branch to edits for maintainers, I can fix the pending issues myself

somethingelseentirely commented 2 months ago

Apparently I can't... I can fix whatever you'd like changed, or feel free to branch off from this one merge that an and then close this PR. :)

Cydhra commented 2 months ago

I can fix whatever you'd like changed

See the unresolved conversation above

somethingelseentirely commented 2 months ago

I feel like out conversation histories convered into different states in githubs database. Can you link me the comments? I don't seem to see them (or am just bad at looking). Maybe it's also not showing up because Irebased everything and force updated my branch.

Cydhra commented 2 months ago

https://github.com/Cydhra/vers/pull/12/files#diff-8877605da1871659d01cbad275a6f7869a0e0c1aead27ec205b22ecaf3e173c0R2023 needs to be 1.5.1, not 0.1.6