-
Notifications
You must be signed in to change notification settings - Fork 139
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 CoreFloat trait #32
Conversation
This is a subset of the `Float` trait, but works with `no_std`. Some code was simplified by using `CoreFloat`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you mention this as an alternative in the Features section of README.md?
One bikeshed possibility is FloatCore
, to be a little more consistent with FloatConst
, but I can go either way. Choose whichever name you prefer. :)
src/float.rs
Outdated
pub trait CoreFloat: Num + Neg<Output = Self> + PartialOrd + Copy { | ||
/// Returns positive infinity. | ||
#[inline] | ||
fn infinity() -> Self; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[inline]
has no effect without a function body -- see rust-lang/rust#47475. That should be moved to the actual implementations.
src/lib.rs
Outdated
@@ -26,7 +26,7 @@ use core::fmt; | |||
pub use bounds::Bounded; | |||
#[cfg(feature = "std")] | |||
pub use float::Float; | |||
pub use float::FloatConst; | |||
pub use float::{CoreFloat, FloatConst}; | |||
// pub use real::Real; // NOTE: Don't do this, it breaks `use num_traits::*;`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may have the same concern as Real
did here. If someone does use num_traits::*
, then the common methods between Float
and CoreFloat
may be ambiguous. Would you mind if we didn't re-export this in the root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure supporting wildcard imports is worthwhile. If you want to make sure not to break them, you can't add any new functions or types. However, in this case I guess it is easy enough to avoid that problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure either. I guess a wildcard making us self-conflicting is a worse than having a conflict with some unrelated crate though.
This avoids breaking `use num_traits::*`.
I think I addressed all your comments. |
Looks great, thanks! bors r+ |
Build succeeded |
@vks I just noticed that this doesn't require |
Yes, I did not need it. Is there a reason to require it?
…On Thu, Feb 8, 2018, 00:44 Josh Stone ***@***.***> wrote:
@vks </~https://github.com/vks> I just noticed that this doesn't require
NumCast like Float does. Was that intentional?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#32 (comment)>,
or mute the thread
</~https://github.com/notifications/unsubscribe-auth/AACCtDRlamQQl55Ki6otN-CVVtmMJeUvks5tSjVqgaJpZM4R3nbo>
.
|
It's a benefit for those who use the trait too. It was there long before
Float itself needed it.
I think I'm going to add a few more methods from Float that are feasible
with no_std, before it's published and becomes a breaking change. I can add
NumCast as I do that...
|
This is a subset of the
Float
trait, but works withno_std
.Some code was simplified by using
CoreFloat
.