Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove redundant bits_per_element #12

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

somethingelseentirely
Copy link
Contributor

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 Cydhra added the enhancement New feature or request label Aug 31, 2024
@Cydhra
Copy link
Owner

Cydhra commented Aug 31, 2024

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
Copy link
Contributor Author

somethingelseentirely commented Aug 31, 2024

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
Copy link
Owner

Cydhra commented Aug 31, 2024

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
Copy link
Contributor Author

Just rebased it onto the master real quick 😄

@Cydhra Cydhra self-assigned this Sep 2, 2024
@Cydhra
Copy link
Owner

Cydhra commented Sep 8, 2024

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

@somethingelseentirely
Copy link
Contributor Author

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
Copy link
Owner

Cydhra commented Sep 9, 2024

I can fix whatever you'd like changed

See the unresolved conversation above

@somethingelseentirely
Copy link
Contributor Author

somethingelseentirely commented Sep 9, 2024

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
Copy link
Owner

Cydhra commented Sep 9, 2024

@Cydhra Cydhra merged commit c99a18e into Cydhra:master Sep 10, 2024
1 check passed
@somethingelseentirely somethingelseentirely deleted the dev_reduntant_len branch September 10, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants