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

Double safety check for Xorshift bit values. #1404

Merged
merged 2 commits into from Jul 9, 2013

Conversation

WebDrake
Copy link
Contributor

@WebDrake WebDrake commented Jul 9, 2013

Add a little bit of extra robustness/future-proofing to the design of Xorshift. It's a check that should have been in my previous pull request but that I overlooked.

@monarchdodra
Copy link
Collaborator

What i originally meant is that while double checks are more robust, if the second check fails, then it is a phobos internal error, and should read and be diagnosed as such: format("Phobos Error: %s bits not implemented", bits). Or format("Phobos Error: Failed to implement XOrShift for %s bits", bits).

As is written, the user wouldn't be able to tell if he made a wrong code call, or if the implementation made an error.

But this is mostly a rhetorical issue, since this is simple code. But as a rule of thumb, I think a lot more static asserts should simply read as Phobos Error.

@WebDrake
Copy link
Contributor Author

WebDrake commented Jul 9, 2013

I actually deliberately designed the messages to be different for each case, but they are subtle differences. We have 3 possible sources of error we need to catch:

  • a user tries to instantiate an XorshiftEngine with an unsupported bit value -- Error I.
  • a developer adds an extra bit value to the allowed list, but forgets to add instantiation for seeds_ - Error II.
  • a developer adds an extra bit value and instantiation, but forgets to add an update rule in popFront() - Error III (this case).

So, I'll tweak all the error messages accordingly, and make use of the "Phobos Error" message you suggest.

@WebDrake
Copy link
Contributor Author

WebDrake commented Jul 9, 2013

Here you go. I didn't make use of format() or text() to create the error message string -- that can be corrected if you prefer.

@monarchdodra
Copy link
Collaborator

Here you go. I didn't make use of format() or text() to create the error message string -- that can be corrected if you prefer.

It's all CTFE, so it shouldn't really make any difference.

Looks good to me.

@WebDrake
Copy link
Contributor Author

WebDrake commented Jul 9, 2013

Tests passed ... ping? :-)

@ghost ghost assigned repeatedly Jul 9, 2013
@repeatedly
Copy link
Member

After current testing passed, I will merge.

@WebDrake
Copy link
Contributor Author

WebDrake commented Jul 9, 2013

Thanks! :-)

repeatedly added a commit that referenced this pull request Jul 9, 2013
Double safety check for Xorshift bit values.
@repeatedly repeatedly merged commit b8531d4 into dlang:master Jul 9, 2013
@WebDrake WebDrake deleted the xorshift branch July 9, 2013 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants