Fix shuffle() function errors #65
Merged
Conversation
- `rand_max` now represents the greatest value returned by the RNG, instead of the length of the range. - Loop condition fixed to stop infinite loop. - Comments updated
Thanks for the comments @JustinDrake! I've added my responses and will await further direction from the repository maintainers. |
Thanks for the feedback. I will implement the following changes:
I could use additional direction regarding |
I'm cool with an assert in the spec code as long as there is a comment about why it is there and how it is up to a client on how to proceed. Similar to the commented assert in If we want to remove asserts entirely (there are many), we can do it in a separate issue. |
- Add `rand_bytes` - Change `for` loop condition for readability and generality. - Ensure consistency of comment spacing - Update comments
All updates have been pushed :) |
lgtm! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
I updated the
shuffle()
function as discussed in #55.The following changes were implemented:
sample_max
was made distinct fromrand_max
.rand_max
now represents the greatest value returned by the RNGinstead of the length of the range. This is consistent with the cstdlib RAND_MAX constant.
while
condition fixed to stop infinite loop.After fixing (2) I noticed that there was an error in generating
sample_max
caused byrand_max
being too high. This affects the output of the shuffle. All clients should update their shuffler if this PR is accepted.I have used the shuffling_sandbox to fuzz this code against my reference implementation.