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 no_std build support #208

Merged
merged 1 commit into from
Dec 17, 2017
Merged

Add no_std build support #208

merged 1 commit into from
Dec 17, 2017

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Dec 14, 2017

It would be nice to get this merged. I went ahead and modified @jethrogb's #108 with extra code from my master branch.

This misses the core_io option; the difference is that ReadRng is not available. I don't consider this a big deal since it's quite easy to re-implement if need be, though if there is significant demand it could be added.

Any objections? It's a shame so many features must be disabled, but there's not much we can do about them. Possibly the f64 distributions could be enabled with some workaround; I'm not sure why exp and log are not in core since the intrinsics are; I won't hold this PR up for that though.

Based on @jethrogb's core branch and my master branch
Plus some extra notes in README
@pitdicker
Copy link
Contributor

I'm not sure why exp and log are not in core since the intrinsics are

I was also looking into that today 😄. But the intrinsics rely on the C library libm. We could rely on https://github.com/nagisa/math.rs as an alternative, but it does not yet have all the functions the distributions need.

Copy link
Contributor

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

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

Given that your master branch already has the no_std support, most of the effort was already done, right? I suppose if it builds, the changes are good.

Does it make sense to include the CI configuration from https://github.com/dhardy/rand/ with this PR or soon, to prove it compiles and keeps doing so?

I agree about not making ReadRng available at the moment. Let's wait until there is actual demand before deciding to add an (optional) dependency or splitting it off into a separate crate.

generators with fresh seeds (user must provide entropy)
- `thread_rng`, `weak_rng` and `random` are all disabled
- exponential, normal and gamma type distributions are unavailable
since `exp` and `log` functions are not provided in `core`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: sqrt and powf are also necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

They are also fairly easy to derive from the above functions.

@dhardy
Copy link
Member Author

dhardy commented Dec 14, 2017

I think I've implemented this no_std stuff 3 or 4 times now. Mostly copy/paste here (that was easier than trying to do a complex merge).

I'm not really sure about the CI stuff; in theory the extra platform tests are useful (the big endian ones in particular have caught some issues), but it's also a lot of extra work for Travis for limited gain. It should be a separate PR though (go ahead yourself if you like).

@pitdicker
Copy link
Contributor

It should be a separate PR though (go ahead yourself if you like).

I will keep it in mind. Honest interest: I don't really understand the direction/strategy you want to go at the moment (but that is more for the discussion in #205)

@dhardy
Copy link
Member Author

dhardy commented Dec 15, 2017

I want to get most of the stuff in my branch merged, but it's a huge inter-related thing, so not fair to ask people to review that. Plus there's some stuff in there that maybe shouldn't be merged. So I'm creating separate PRs. Not sure what actual releases will happen however.

@pitdicker
Copy link
Contributor

That sounds like a lot of work, but is probably the responsible way.

This was referenced Dec 15, 2017
@dhardy dhardy merged commit 37216c4 into rust-random:master Dec 17, 2017
@dhardy dhardy deleted the core branch December 17, 2017 17:54
pitdicker pushed a commit to pitdicker/rand that referenced this pull request Apr 4, 2018
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.

2 participants