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 a trait for floating-point constants #220

Merged
merged 1 commit into from
Aug 16, 2016
Merged

Add a trait for floating-point constants #220

merged 1 commit into from
Aug 16, 2016

Conversation

IvanUkhov
Copy link
Contributor

The pull request is to address issue #194. In order to keep the library organized, I’ve introduced a new trait for the new functionality. The trait is supposed to closely follows the consts module from the standard library. There are at least the following three open questions:

  1. What should the name of the trait be? Currently, it’s Constant; an alternative is Consts.
  2. What should the names of the getters be? Currently, they are lower-case versions of the constants defined in the consts module. Note that Float provides log2 and log10, whereas LOG_2 and LOG_10 get translated into log_2 and log_10, respectively.
  3. Should Float require the new trait? Or should it be left up to the user?

Please let me know what you think. Thank you!

Regards,
Ivan

@cuviper
Copy link
Member

cuviper commented Aug 13, 2016

Thanks for this!

What should the name of the trait be? Currently, it’s Constant; an alternative is Consts.

I don't like either name if we export the at the top level of traits -- it's too general. Think also about how people should be expected to invoke this. Something like num_traits::float::Constant::e()? Or perhaps use num_traits::FloatConsts; then FloatConsts::e()?

What should the names of the getters be? Currently, they are lower-case versions of the constants defined in the consts module. Note that Float provides log2 and log10, whereas LOG_2 and LOG_10 get translated into log_2 and log_10, respectively.

I guess the ones you mean are ln_2 and ln_10, so the names aren't quite so close. But I think we could justify all-caps function names here to mimic true constants. (Just add an #[allow] for whatever lint complains.) I don't feel strongly either way though.

Should Float require the new trait? Or should it be left up to the user?

The nice thing about having them totally separate is that this isn't a breaking change to add it. If Float requires it, then any (hypothetical) outside implementation would be broken by not having it.

@IvanUkhov
Copy link
Contributor Author

Question 1. I agree that the name Constant is too general when considered in the context of the root module. I re-exported the trait in order to follow the conventions that I saw in the code: you keep all the modules public but at the same time re-export all their contents. If we don’t re-export Constant, we won’t have this problem; it’ll only be accessible through float.

Let’s see how different versions might look:

use num_traits::float::{Float, Const};
fn foo1<T>(bar: T) -> T where T: Float + Const {
    bar + T::pi() // or Const::pi()
}

use num_traits::float::{Float, Constant};
fn foo2<T>(bar: T) -> T where T: Float + Constant {
    bar + T::pi() // or Constant::pi()
}

use num_traits::{Float, FloatConst};
fn foo3<T>(bar: T) -> T where T: Float + FloatConst {
    bar + T::pi() // or FloatConst::pi()
}

use num_traits::{Float, FloatConsts};
fn foo4<T>(bar: T) -> T where T: Float + FloatConsts {
    bar + T::pi() // or FloatConsts::pi()
}

use num_traits::{Float, FloatConstant};
fn foo5<T>(bar: T) -> T where T: Float + FloatConstant {
    bar + T::pi() // or FloatConstant::pi()
}

use num_traits::{Float, FloatConstants};
fn foo6<T>(bar: T) -> T where T: Float + FloatConstants {
    bar + T::pi() // or FloatConstants::pi()
}

To me personally, singular forms make more sense. Regarding with or without float::, probably it’s better to make the new trait directly accessible, just like all other items in the crate; hence, Float should be in the name of the new trait. In this case, FloatConstant is getting a bit too long, so I’d personally choose FloatConst (foo3).

Question 2. Yes, you’re right. Sorry for the confusion. I very much like your idea about all caps.

Question 3. Yes, it’s better to keep Float independent of the constants.

You’re in charge here, and all the choices are entirely up to you. Please tell me what changes should be made to the pull request. Thank you.

@cuviper
Copy link
Member

cuviper commented Aug 14, 2016

Somehow I forgot that you can use T::method() for any trait that T implements, even though I write like that all the time. It certainly makes this more ergonomic.

Sounds like a reasonable consensus: FloatConst as an independent trait with uppercase names.

@IvanUkhov
Copy link
Contributor Author

I’ve updated the pull request. Thank you.

@cuviper
Copy link
Member

cuviper commented Aug 16, 2016

Looks good, thanks!

@homu r+ 01aad70

@homu homu merged commit 01aad70 into rust-num:master Aug 16, 2016
homu added a commit that referenced this pull request Aug 16, 2016
Add a trait for floating-point constants

The pull request is to address issue #194. In order to keep the library organized, I’ve introduced a new trait for the new functionality. The trait is supposed to closely follows the [`consts`](https://doc.rust-lang.org/std/f64/consts/index.html) module from the standard library. There are at least the following three open questions:

1. What should the name of the trait be? Currently, it’s `Constant`; an alternative is `Consts`.
2. What should the names of the getters be? Currently, they are lower-case versions of the constants defined in the `consts` module. Note that `Float` provides `log2` and `log10`, whereas `LOG_2` and `LOG_10` get translated into `log_2` and `log_10`, respectively.
3. Should `Float` require the new trait? Or should it be left up to the user?

Please let me know what you think. Thank you!

Regards,
Ivan
@homu
Copy link
Contributor

homu commented Aug 16, 2016

⚡ Test exempted - status

remexre pushed a commit to remexre/num that referenced this pull request Jun 1, 2017
remexre pushed a commit to remexre/num that referenced this pull request Jun 1, 2017
remexre pushed a commit to remexre/num that referenced this pull request Jun 1, 2017

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…arser

Fix rust-num#220 (invalid enum parser)
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.

None yet

3 participants