search

isaacholt100 / bnum

Arbitrary, fixed size numeric types that extend the functionality of primitive numeric types in Rust.
https://crates.io/crates/bnum
Other
67 stars 8 forks source link

`u8_digit` feature is non-additive #7

Closed jeremyBanks closed 1 year ago

jeremyBanks commented 1 year ago

First, thanks for this library! I've been looking for larger integer types last week and this looks much nicer than what I'd been considering.

I just wanted to note that this crate's u8_digit feature seems to be non-additive, as described in the Cargo book:

When a dependency is used by multiple packages, Cargo will use the union of all features enabled on that dependency when building it. This helps ensure that only a single copy of the dependency is used. See the features section of the resolver documentation for more details. [...] A consequence of this is that features should be additive. That is, enabling a feature should not disable functionality, and it should usually be safe to enable any combination of features. A feature should not introduce a SemVer-incompatible change.

It's not a big deal, but if this crate ends up being widely-adopted it could result in confusing errors if different dependencies end up using different digit sizes, even if they don't interact, so it might be better to avoid if practical.

If the Digit type were only internal this wouldn't be a problem, so perhaps it would be possible to avoid exposing it directly. It doesn't seem to appear in too many places. Maybe you could instead have separate functions like from_u8_digits and from_u64_digits. I'm not sure if that would satisfy your design goals or not.

In any case, thanks again, I expect to get a lot of use out of this library. 🙂

isaacholt100 commented 1 year ago

@jeremyBanks Thanks very much for pointing this out! This is my first time publishing a Rust library so I was not aware of this Cargo feature behaviour. I can see that in its current state it could cause some issues for dependencies in the future. Ideally I would like to keep the option of using u8 as the digit instead of u64 to allow for better memory optimisation, so I will have a think about what the best option would be to retain this functionality while fixing the non-additivity issue.

I originally started writing this library using a generic Digit type in the BUint type signature, but switched to a type alias because it was slightly easier to implement and allowed methods to be const. However, I suppose that I could refactor the code to use a generic Digit type. The penalty for this would be that nearly all methods could only be const on nightly.

Another solution could be to use macros to generate two unsigned integer types and two equivalent signed integer types. One would be stored as an array of u64 digits while the other would be an array of u8 digits. This has the benefit of keeping the const behaviour and would probably require less refactoring.

Thanks again for informing me of this, this would be an important decision so I may take some time to figure out the best solution. I'm glad that the library will be useful to you! (Although I should just make clear that 33 and 65 bit integers are not currently available as the bit width needs to be a multiple of 64 or 8 at the moment.)

isaacholt100 commented 1 year ago

Fixed. Now there are 4 signed and 4 unsigned integer types, each with a different digit size, meaning the u8_digit feature is no longer necessary and has been removed. I will create a new release with these changes soon.