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

Add BigUint::{is, next}_power_of_two() #212

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PatrickNorton
Copy link
Contributor

@PatrickNorton PatrickNorton commented Aug 8, 2021

These are two more integer methods we don't have on BigUint, which would be nice to have.

Unresolved questions:

  • Currently, next_power_of_two takes self by value instead of reference, to prevent unnecessary clones. Is that the right decision?
  • If we decide to have next_power_of_two take self by reference, should we add a to_next_power_of_two method that takes it by value?

@cuviper
Copy link
Member

cuviper commented Aug 28, 2021

On naming, there are these guidelines between as_, to_, and into_:
https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

There is also the precedent of the next_power_of_two method on the primitive integers, which is not prefixed at all and takes self by value. But those integers are trivially Copy, so they almost never bother with borrowed &self.

There are also a lot of num-traits methods that take &self for the sake of types like BigUint, even when the primitive types would take self by value in almost every case.

So if we want to support both owned and borrowed, I could see either of these options working well:

  • next_power_of_two(self) and to_next_power_of_self(&self)
  • next_power_of_two(&self) and into_next_power_of_self(self)

@PatrickNorton
Copy link
Contributor Author

I'd think it's better to go with the by-value as the default; it seems better to promote the more efficient method as the default and let people switch out of it when they need it.

@PatrickNorton
Copy link
Contributor Author

Is there anything still blocking this being merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants