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

Update the random_device implementation #34

Closed
wants to merge 1 commit into from

Conversation

jeking3
Copy link
Contributor

@jeking3 jeking3 commented Dec 7, 2017

This is an adaptation of boostorg/uuid#53, with the intention to keep random_device as the entropy provider by moving the relevant code into Boost.Random.

This pull request updates the random_device implementation so that it:

  • Supports more platforms optimally, for example CloudABI, OpenBSD, and Windows UWP
  • Is easier to maintain, as each platform's implementation is in a separate file
  • Removes the library dependency on Boost.System
  • Is header-only, and thus makes Boost.Random header-only
  • Is well-tested for happy and sad paths

Removes the token-based random_device explicit constructor. This came up as a security issue in a code review of a similar pull request in Boost.Uuid. Further, the token no longer makes sense with respect to the code here.

Expanded the documentation in the random_device header.

Adds a new exception "entropy_error" to handle errors getting entropy.

Removed the detail auto_link implementation inside Boost.Random as it is no longer necessary - the one in Boost.Config is sufficient.

Updated the documentation.

Updated the random_device::generate method with additional checks based on code reviews for similar code in another boost project.

Also added a top-level Jamfile that builds the "example" sub-directory with each build, as one of the examples needed to be updated in order to build. This will prevent rot in the example directory.

Note: Other libraries that link against Boost.Random (like Boost.Uuid) will fail to build until they stop trying to link against Boost.Random.

CI Build Results

https://ci.appveyor.com/project/jeking3/random/build/1.0.47-develop
https://travis-ci.org/jeking3/random/builds/313178800

This fixes #20
This fixes #22

* Supports more platforms optimally, for example CloudABI, OpenBSD, and Windows UWP
* Is easier to maintain as each platform's implementation is in a separate file
* Removes the library dependency on Boost.System
* Is header-only, and thus makes Boost.Random header-only
* Is well-tested for happy and sad paths

Removes the token-based random_device explicit constructor.

Adds a new exception "entropy_error" to handle errors getting entropy.

Removed the detail auto_link implementation inside Boost.Random as it is no longer
necessary - the one in Boost.Config is sufficient.

Also added a top-level Jamfile that builds the example subdirectory with each
build, as one of the examples needed to be updated in order to build.  This will
prevent rot in the example directory.

Note: Other libraries that link against Boost.Random (like Boost.Uuid) will fail
to build until they stop trying to link against Boost.Random.

This fixes boostorg#20
This fixes boostorg#22
@swatanabe
Copy link
Collaborator

swatanabe commented Dec 7, 2017 via email

@jeking3
Copy link
Contributor Author

jeking3 commented Dec 8, 2017

Thanks @swatanabe - is this something that has a chance of being merged, or should I drop it?

I modified the example on the documentation page because it is misleading. The example as stated will always produce the same random number sequence from the default-seeded mersenne twister, and I find that to be a misleading or poor example for others to follow. Do you disagree?

I'm trying to put the code for entropy generation into the right place... It's not my intention to upset you here. I just want Boost.Uuid out of the business of entropy generation, and can provide the additional benefits listed.

@swatanabe
Copy link
Collaborator

swatanabe commented Dec 8, 2017 via email

@jeking3
Copy link
Contributor Author

jeking3 commented Dec 8, 2017

Okay, so, if I restore the example, restore the library, eliminate the entropy_error, fix the documentation to reflect that, and put the random provider implementations into a library then there is a possibility this can be considered for merge - correct? Will you accept if the cpp file just includes those two headers, and I move the .ipp files into src with the cpp that includes them, or do you want me to make one amalgamation of all the files with preprocessor barriers? Let me know, we can get this done. I'm motivated to make it happen.

@jeking3
Copy link
Contributor Author

jeking3 commented Dec 8, 2017

Wait, nevermind, my work in Boost.Uuid cannot be rerouted here if this implementation relies on a library. I will drop this work.

@jeking3 jeking3 closed this Dec 8, 2017
@swatanabe
Copy link
Collaborator

swatanabe commented Dec 8, 2017 via email

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