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

Implement num-traits. #19

Open
boomshroom opened this issue Jan 11, 2019 · 8 comments
Open

Implement num-traits. #19

boomshroom opened this issue Jan 11, 2019 · 8 comments

Comments

@boomshroom
Copy link

In addition to the various traits in std, implementing the traits from num-traits would enable writing algorithms and data structures that are polymorphic on the the number of bits. While any of them would be useful, PrimInt would be incredibly useful, but it has a very large number of dependencies and has methods that don't make a huge amount of sense for integers with a bit-width that isn't a multiple of a byte, namely {to,from}_{be,le} and swap_bytes.

As most of the traits depend on operator traits in the standard library, this is blocked by #11.

@kjetilkjeka
Copy link
Collaborator

Thanks for bringing this to attention.

My main concern is that num_traits is not stable, and exposing this interface publicly would have consequences for the stability of this library.

I know there was a lot of numeric traits in core/std before 1.0, is the aim to bring these into the std lib again? A bit like what's currently happening to Future.? This would be the ideal situation.

Maybe it would be valuable to expose such interface in an unstable nightly-only feature?

@boomshroom
Copy link
Author

My understanding is that num-traits, like the rest of the num family of crates was fairly stable. That said, I would understand completely if you would rather have it be locked behind a feature flag.

@kjetilkjeka
Copy link
Collaborator

Since the types from this crate are analog to types from the stdlib, there are not any reason that this crate can have the same stability guarantees as the stdlib once released in 1.0. For this to be possible, this crate should preferably not have any public dependencies at all. And at least only have public dependencies of other crates that share the same stability goals.

As I said, I would be happy to include implementation of these types behind some sort of nightly only unstable feature flag (that won't block the release of 1.0). Some sort of vision behind how to later stabilize it would be preferred.

@boomshroom
Copy link
Author

I currently have it behind a feature gate. One question I'd like to ask is whether or not you'd prefer to have trait impls for things like num-traits and serde in their own files, or if it would be fine to put them alongside the existing implementations. Right now, I have what I can write inline with the base implementations. That said, due to #11, I can only implement 6 of the traits, 3 of which already have corresponding stable functions, another is Zero. I've also added to conversion.rs implementations for ToPrimitive and FromPrimitive in a fairly straight forward manner. Being fallible conversions, I did have to write my own macro for them.

@kjetilkjeka
Copy link
Collaborator

I currently have it behind a feature gate.

Great, just remember that (in Rust spirit) this unstable feature should be nightly only.

One question I'd like to ask is whether or not you'd prefer to have trait impls for things like num-traits and serde in their own files, or if it would be fine to put them alongside the existing implementations.

I think it would be fine to keep them in lib.rs as long as they're trivial and doesn't clutter the rest of the file. However, the advantage of keeping it in a file of its own is the ability to add the conditional compilation directive only on the module. Use your best judgment and I will have a closer look once you open a PR.

That said, due to #11, I can only implement 6 of the traits, 3 of which already have corresponding stable functions, another is Zero.

Which traits would have to be implemented for a full implementation?

I've also added to conversion.rs implementations for ToPrimitive and FromPrimitive in a fairly straight forward manner. Being fallible conversions, I did have to write my own macro for them.

TryFrom and TryInto is on the verge of being stabilized, maybe implementation for these traits could be included in the unstable feature and stabilized once the conversion traits are?

@boomshroom
Copy link
Author

Given that the num crates don't require nightly makes me reluctant to allow their traits only on nightly.

As for which traits are necessary, almost all of them. A few like NumOps have a blanket implementation if all the corresponding traits from core:ops are implemented. Each trait has its own requirements, but most depend on the Num trait which itself depends on the aforementioned NumOps trait. The One trait should be simple, but since it's supposed to specifically be the multiplicative identity, it depends on an implementation of core::ops::Mul.

@boomshroom
Copy link
Author

boomshroom commented Jan 15, 2019

Since you're worried about stability, I will mention that the num crate has nearly 3.5 million downloads while num-traits specifically has an additional 3 million on top of that.

num re-exports num-traits, so I subtracted num's count first. The total downloads for num-traits was 6.6 million

@kjetilkjeka
Copy link
Collaborator

Since you're worried about stability, I will mention that the num crate has nearly 3.5 million downloads while num-traits specifically has an additional 3 million on top of that.

num-traits broke it's API less than a year ago (Feb 7, 2018), since it's still not in 1.0 this will happen again. Since these traits require a "public" dependency including num_traits as a deps (without putting it behind unstable nightly only feature gate) will effectively block this crate from being released in 1.0 and putting it on num_traits release schedule. This is not something I would like to do.

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

No branches or pull requests

2 participants