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

impl std::error::Error for rand_distr::*Error. #919

Merged
merged 4 commits into from Dec 31, 2019

Conversation

kennytm
Copy link
Contributor

@kennytm kennytm commented Dec 22, 2019

Also re-exported BernoulliError into rand_distr.

@burdges
Copy link
Contributor

burdges commented Dec 22, 2019

Requires feature gates for std.

I wrote posted some thought about an error trait without alloc btw:
rust-lang/rfcs#2820 (comment)

@kennytm
Copy link
Contributor Author

kennytm commented Dec 22, 2019

@burdges rand_distr does not have a gate for std because it uses std-only methods like f64::powi (see rust-lang/rfcs#2505).

Unlike most Rand crates, rand_distr does not currently support no_std.

Until we get non-std-support of these math functions there's no point to feature gate std::error::Error.

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.

Could you add a note to the changelog please? https://rust-random.github.io/book/contributing.html#pull-requests

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.

Thanks; mostly this looks good but perhaps a couple of doc strings should be tweaked.

rand_distr/src/dirichlet.rs Show resolved Hide resolved
rand_distr/src/pert.rs Outdated Show resolved Hide resolved
src/distributions/bernoulli.rs Outdated Show resolved Hide resolved
@dhardy dhardy mentioned this pull request Dec 30, 2019
3 tasks
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.

Looks good.

Sorry to add yet another thing to this PR, but I just realised we don't catch the Bernoulli::new(NaN) case. I don't think this is classified as a breaking change, but is worth a mention in the changelog. Can I get you to add it to this PR? Thanks.

@dhardy
Copy link
Member

dhardy commented Dec 31, 2019

That also applies to from_ratio(0, 0). Okay, this is technically a different change, so should be a different PR.

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