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

Fix Issue 17862 - std.random.XorshiftEngine.min is wrong when bits == 32 #5746

Merged
merged 1 commit into from Mar 23, 2018

Conversation

n8sh
Copy link
Member

@n8sh n8sh commented Sep 27, 2017

XorshiftEngine.min is defined as 0 regardless of template parameters
but an XorshiftEngine cannot produce a value of zero if its internal
state has the same number of bits as the output element type.

@n8sh n8sh requested a review from wilzbach as a code owner September 27, 2017 06:48
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @n8sh! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close Bugzilla Description
17862 std.random.XorshiftEngine.min is wrong when bits == 32

@JackStouffer
Copy link
Member

Pinging our std.random experts @wilzbach @WebDrake @andralex

@quickfur quickfur added the @andralex Approval from Andrei is required label Jan 26, 2018
@quickfur
Copy link
Member

Code looks OK to me. But I'll defer to others who know better about PRNGs.

Also, needs rebase.

XorshiftEngine.min is defined as 0 regardless of template parameters
but an XorshiftEngine cannot produce a value of zero if its internal
state has the same number of bits as the output element type.
@n8sh
Copy link
Member Author

n8sh commented Jan 30, 2018

I'll defer to others who know better about PRNGs.

Here's the code for front:

    static if (bits == 192)
        return value_;
    else
        return seeds_[size - 1];

Here's the popFront code for the seeds_.length == 1 case:

    UIntType temp;

    static if (bits == 32)
    {
        temp      = seeds_[0] ^ (seeds_[0] << a);
        temp      = temp ^ (temp >> b);
        seeds_[0] = temp ^ (temp << c);
    }
    else
        /* all other branches are irrelevant */

For unsigned types you can see x ^ (x << k) and x ^ (x >> k) are zero only when x is zero, so when seeds_.length == 1 the only way for front to be zero after popFront is if it were also zero before popFront.

The constructor and the seed method call sanitizeSeeds which disallows 0 from appearing in seeds_, and seeds_ has a non-zero initializer, so when seeds_.length == 1 there is no way to get front == 0 without memory corruption.

Also, needs rebase.

Rebased.

@andralex andralex added auto-merge and removed @andralex Approval from Andrei is required labels Mar 22, 2018
@WebDrake
Copy link
Contributor

Oh cripes, sorry for forgetting about this one. It looks reasonable, and I'd expect the impact to be minimal.

@dlang-bot dlang-bot merged commit cc256d8 into dlang:master Mar 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants