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

Make Uniform constructors return a result #1229

Merged
merged 10 commits into from
Feb 6, 2023

Conversation

vks
Copy link
Collaborator

@vks vks commented Apr 22, 2022

  • This is a breaking change.
  • The new error type had to be made public, otherwise Uniform could
    not be extended for user-defined types by implementing
    UniformSampler.
  • rand_distr was updated accordingly.
  • Also forbid unsafe code for crates where none is used.

Fixes #1195, #1211.

vks added 2 commits April 22, 2022 22:57
This helps tools like `cargo geiger`.
- This is a breaking change.
- The new error type had to be made public, otherwise `Uniform` could
  not be extended for user-defined types by implementing
  `UniformSampler`.
- `rand_distr` was updated accordingly.
- Also forbid unsafe code for crates where none is used.

Fixes rust-random#1195, rust-random#1211.
src/distributions/uniform.rs Outdated Show resolved Hide resolved
src/distributions/uniform.rs Outdated Show resolved Hide resolved
src/distributions/uniform.rs Outdated Show resolved Hide resolved
src/distributions/uniform.rs Outdated Show resolved Hide resolved
src/distributions/uniform.rs Outdated Show resolved Hide resolved
src/distributions/uniform.rs Outdated Show resolved Hide resolved
src/distributions/uniform.rs Outdated Show resolved Hide resolved
@dhardy
Copy link
Member

dhardy commented Aug 3, 2022

@vks mind if I ping you on the status of this?

@vks
Copy link
Collaborator Author

vks commented Aug 3, 2022

@dhardy I'll try to have a look this Sunday.

@dhardy dhardy added the D-changes Do: changes requested label Nov 9, 2022
@dhardy
Copy link
Member

dhardy commented Dec 6, 2022

Skimming the above it looks like this should make sample_single{,_inclusive} return a Result too, plus a few small changes. @vks are you still okay to do this or shall I make a patch?

@vks
Copy link
Collaborator Author

vks commented Dec 6, 2022

I have a patch that is almost ready.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

It's worth noting here that this PR introduces .unwrap() in several distributions where it should be statically-verifiable that the range is non-empty, but of course the language doesn't let us do that.

Comment on lines +265 to +276
/// Usually users should not call this directly but prefer to use
/// [`Uniform::new`].
fn new<B1, B2>(low: B1, high: B2) -> Result<Self, Error>
where
B1: SampleBorrow<Self::X> + Sized,
B2: SampleBorrow<Self::X> + Sized;

/// Construct self, with inclusive bounds `[low, high]`.
///
/// Usually users should not call this directly but instead use
/// `Uniform::new_inclusive`, which asserts that `low <= high` before
/// calling this.
fn new_inclusive<B1, B2>(low: B1, high: B2) -> Self
/// Usually users should not call this directly but prefer to use
/// [`Uniform::new_inclusive`].
fn new_inclusive<B1, B2>(low: B1, high: B2) -> Result<Self, Error>
Copy link
Member

Choose a reason for hiding this comment

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

There's not really a reason users shouldn't call these directly now (IIRC we used to have an assert in Uniform::new), though Uniform::new is probably still the more convenient API.

@dhardy
Copy link
Member

dhardy commented Dec 7, 2022

This this conflicts with #1239. @vks would you be so kind?

@vks
Copy link
Collaborator Author

vks commented Dec 11, 2022

I'll try to look into it later this week.

@dhardy
Copy link
Member

dhardy commented Dec 11, 2022

Sure. I used the web interface to make a merge commit, but it may be incorrect.

src/distributions/uniform.rs Outdated Show resolved Hide resolved
@dhardy dhardy merged commit 7d73990 into rust-random:master Feb 6, 2023
@vks vks deleted the uniform-result branch February 6, 2023 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-changes Do: changes requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error handling of distributions::Uniform::new
2 participants