Skip to content

Make Uniform constructors return a result - #1229

Merged
dhardy merged 10 commits into
rust-random:masterfrom
vks:uniform-result
Feb 6, 2023
Merged

Make Uniform constructors return a result#1229
dhardy merged 10 commits into
rust-random:masterfrom
vks:uniform-result

Conversation

@vks

@vksvks commented Apr 22, 2022

Copy link
Copy Markdown
Contributor
  • 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
@vks
Forbid unsafe code in crates without unsafe code
f9bce60
This helps tools like `cargo geiger`.
@vks
Make Uniform constructors return a result
e40f00a
- 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.
Fixesrust-random#1195, rust-random#1211.
dhardy
dhardy reviewed Apr 25, 2022
View reviewed changes
Comment threadsrc/distributions/uniform.rs Outdated
Comment threadsrc/distributions/uniform.rs Outdated
Comment threadsrc/distributions/uniform.rs Outdated
Comment threadsrc/distributions/uniform.rs Outdated
Comment threadsrc/distributions/uniform.rs Outdated
Comment threadsrc/distributions/uniform.rs
Comment threadsrc/distributions/uniform.rs Outdated
@dhardy

dhardy commented Aug 3, 2022

Copy link
Copy Markdown
Member

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

@vks

vks commented Aug 3, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@dhardydhardy added the D-changes Do: changes requested label Nov 9, 2022
@dhardydhardy mentioned this pull request Dec 6, 2022
Closed
@dhardy

dhardy commented Dec 6, 2022

Copy link
Copy Markdown
Member

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

vks commented Dec 6, 2022

Copy link
Copy Markdown
ContributorAuthor

I have a patch that is almost ready.

dhardy
dhardy approved these changes Dec 7, 2022
View reviewed changes

@dhardydhardy left a comment

Copy link
Copy Markdown
Member

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 threadsrc/distributions/uniform.rs
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>

@dhardydhardyDec 7, 2022

Copy link
Copy Markdown
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

dhardy commented Dec 7, 2022

Copy link
Copy Markdown
Member

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

@vks

vks commented Dec 11, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@dhardy

dhardy commented Dec 11, 2022

Copy link
Copy Markdown
Member

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

dhardy
dhardy reviewed Feb 3, 2023
View reviewed changes
Comment threadsrc/distributions/uniform.rs Outdated
@dhardy
dhardy merged commit 7d73990 into rust-random:masterFeb 6, 2023
@vks
vks deleted the uniform-result branch February 6, 2023 17:59
@TheIronBornTheIronBorn mentioned this pull request Feb 8, 2024
Closed
benjamin-lieser pushed a commit to benjamin-lieser/rand that referenced this pull request Feb 5, 2025
@vks
MakeUniformconstructors return a result (rust-random#1229)
00611a8
* Forbid unsafe code in crates without unsafe code
This helps tools like `cargo geiger`.
* Make `Uniform` constructors return a result
- 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.
Fixesrust-random#1195, rust-random#1211.
* Address review feedback
* Make `sample_single` return a `Result`
* Fix benchmarks
* Small fixes
* Update src/distributions/uniform.rs
benjamin-lieser pushed a commit to benjamin-lieser/rand that referenced this pull request Feb 5, 2025
@vks
MakeUniformconstructors return a result (rust-random#1229)
0821c52
* Forbid unsafe code in crates without unsafe code
This helps tools like `cargo geiger`.
* Make `Uniform` constructors return a result
- 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.
Fixesrust-random#1195, rust-random#1211.
* Address review feedback
* Make `sample_single` return a `Result`
* Fix benchmarks
* Small fixes
* Update src/distributions/uniform.rs
@ghost-of-high-heartghost-of-high-heartBot mentioned this pull request Apr 5, 2026
Closed
This was referenced Jul 27, 2026
Closed
Open
@vxfemboyvxfemboy mentioned this pull request Jul 29, 2026
Open
1 task
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhardydhardydhardy approved these changes

Assignees

No one assigned

Labels

D-changesDo: changes requested

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error handling of distributions::Uniform::new

2 participants

@vks@dhardy