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 From<RangeInclusive> for Uniform #566

Merged
merged 2 commits into from
Jul 27, 2018

Conversation

vks
Copy link
Collaborator

@vks vks commented Jul 23, 2018

Fixes #556.

@vks
Copy link
Collaborator Author

vks commented Jul 23, 2018

If we decide against merging this, I think f3c8f85 should be cherry-picked onto master.

let r = Uniform::from(2.0f64..=7.0);
assert_eq!(r.inner.low, 2.0);
assert!(r.inner.scale > 5.0);
assert!(r.inner.scale < 5.0 + 1e15);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should that be 1e-15?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that was indeed a typo.

Cargo.toml Outdated
@@ -25,6 +25,7 @@ alloc = ["rand_core/alloc"] # enables Vec and Box support (without std)
i128_support = [] # enables i128 and u128 support
simd_support = [] # enables SIMD support
serde1 = ["serde", "serde_derive", "rand_core/serde1"] # enables serialization for PRNGs
rust_1_27 = [] # enables RangeInclusive support for Rust >= 1.27
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, why not.

I think this can imply i128_support, but probably not simd_support since not all of that is stable yet(?).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. simd_support is currently broken and being addressed in #565.

@dhardy
Copy link
Member

dhardy commented Jul 24, 2018

Looks good. @pitdicker did you want to review?

And any further comments about adding rust_1_27 as a feature? I'm tempted to just bump the minimum version instead; we could also just postpone this addition. (This is for Rand 0.6, which may be 1-2 months in dev yet...)

@vks
Copy link
Collaborator Author

vks commented Jul 24, 2018

And any further comments about adding rust_1_27 as a feature?

Personally, I don't need it, but I think that some authors of Rand's most popular dependent crates (@cuviper, @BurntSushi) like to avoid bumping the minimal Rust version. My plan was to ship Rand 0.6 without bumping the minimal Rust version (given there is not a very strong motivation) to maximize adoption of 0.6. We could then bump it for 0.7.

On the other hand, rand is usually an optional or dev-only dependency. I'm not sure introducing a rust_1_27 is worth the effort, but then it is not a lot of effort to begin with.

@BurntSushi
Copy link

I don't have any opinions on optional features, but I think bumping the minimum Rust version for a core library like rand should generally be pretty conservative. But that's just my opinion. If you're only going to wait a month or two before pushing to the latest Rust, then I'm just not going to update rand. You might be OK with that though, I honestly don't know.

@dhardy
Copy link
Member

dhardy commented Jul 24, 2018

Which is why I suggested making the Rand version requirement more flexible in BurntSushi/byteorder#127 and rust-lang/regex#500. Although I suppose that may still cause problems since there is no Rust version dependency requirement in Cargo.toml files, thus Cargo may automatically pick the latest version of Rand even on old compilers. However for these two crates it's not actually important since Rand is not a public dependency.

Rust is still a rapidly moving target and many core libraries are still unstable, so compatibility with old language versions is a burden and in my view not very useful, although compatibility over the last six months of Rust releases may be prudent (which is approximately where we are now with 1.22).

@cuviper
Copy link
Contributor

cuviper commented Jul 24, 2018

Regarding a rust_1_27 feature, my suggestion would be to make this a cfg flag emitted by build.rs instead, so it Just Works when possible. I started with just a crate feature for "i128" in num-traits, but some reddit discussion pointed out that this makes it impossible for other crates to auto-detect compiler support, since their build script can't activate features in other crates. With an emitted cfg flag, they can all work together automatically.

@vks
Copy link
Collaborator Author

vks commented Jul 24, 2018

@cuviper That seems like the best option, I'll look into it.

@dhardy dhardy added the X-experimental Type: experimental change label Jul 26, 2018
@vks
Copy link
Collaborator Author

vks commented Jul 26, 2018

I rebased on master, replaced the rust_1_27 feature with build-time detection and squashed the commits.

@dhardy dhardy mentioned this pull request Jul 26, 2018
@dhardy
Copy link
Member

dhardy commented Jul 27, 2018

Only thing I can see against this is the potential for build failures when using older compilers due to implicit feature activation, but this is the same as for inclusion of low..=high syntax into Rust anyway (except requiring one further release). So lets do it.

Only build failure is the SIMD one — so lets go!

@dhardy dhardy merged commit ba3d273 into rust-random:master Jul 27, 2018
@vks
Copy link
Collaborator Author

vks commented Jul 27, 2018

Only thing I can see against this is the potential for build failures when using older compilers due to implicit feature activation, but this is the same as for inclusion of low..=high syntax into Rust anyway (except requiring one further release).

How could that happen? The build script checks for the Rust version, so it should never fail on old compilers. (As long as we don't use low..=high in Rand.)

@vks vks deleted the range-inclusive branch July 27, 2018 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
X-experimental Type: experimental change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants