Port "random" builtin to rust#9593
Merged
Merged
Conversation
*No* idea if this is the idiomatic thing to do
Xiretza
reviewed
Feb 19, 2023
Xiretza
left a comment
Xiretza
left a comment
Contributor
There was a problem hiding this comment.
I think I've found an acceptable solution to the integer width problem.
Please also run rustfmt (cargo fmt).
Xiretza
reviewed
Feb 19, 2023
Contributor
|
I hope nobody relies on seeded random values to be stable across fish versions. |
Member
Author
I do not believe it is possible to translate this while keeping the values stable, especially because I don't think the values are stable across C++ implementations? So we couldn't even port what that does. Truth be told I would be surprised if anyone actually used the seeding functionality. So this is simply one change we're gonna have to allow. |
Turns out we can do it without switching to 128-bit wide numbers. Co-authored-by: Xiretza <xiretza@xiretza.xyz>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Repeat of #9585
This ports the "random" builtin, that is the builtin called
randomand not just any random builtin, to rust.The remaining issue here is some weirdness with how the C++ does the numbers - the tests explicitly exercise that you can declare random numbers from the smallest signed long long to the largest with a step size of the difference between the two, and I haven't been able to make that work.This allows the weird signed 64-bit number + an unsigned 64 bit step logic, by doing all the math in 128-bit space.Also the error messages are a bit of a mess in the C++ - a step of "-1" is "invalid integer", but a step of "0" is "STEP must be a non-negative integer" for some reason?
This uses a new crate, rand, which is what seemed to be the recommended course if you needed a seedable RNG, which we do (random 12 seeds the RNG with "12").
Also I needed some way to get wcstoi to tell me it hasn't read the entire token, so I picked a brute-force approach. I would assume there's a more elegant approach?This represents about
98%99% of all the rust code I've ever written, so let's see how much I missed!TODOs: