num-bigint icon indicating copy to clipboard operation
num-bigint copied to clipboard

Add `BigUint::{is, next}_power_of_two()`

Open PatrickNorton opened this issue 3 years ago • 3 comments

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

Unresolved questions:

  • [x] Currently, next_power_of_two takes self by value instead of reference, to prevent unnecessary clones. Is that the right decision?
  • [x] 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?

PatrickNorton avatar Aug 08 '21 14:08 PatrickNorton

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)

cuviper avatar Aug 28 '21 00:08 cuviper

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 avatar Aug 28 '21 01:08 PatrickNorton

Is there anything still blocking this being merged?

PatrickNorton avatar Oct 21 '21 23:10 PatrickNorton