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

this._z is used uninitialized in MersenneTwisterEngine.opCall #6

Closed
WebDrake opened this issue Dec 14, 2016 · 7 comments
Closed

this._z is used uninitialized in MersenneTwisterEngine.opCall #6

WebDrake opened this issue Dec 14, 2016 · 7 comments

Comments

@WebDrake
Copy link
Contributor

The _z field of the MersenneTwisterEngine is uninitialized on struct creation:

The constructor does not touch it directly, but calls popFront(), where in this first call, _z is used to set the value of z without _z itself having been initialized first:

Presumably the intention/expectation is that _z should be initialized to data[index] in the constructor?

@WebDrake
Copy link
Contributor Author

Note that the effect is unobservable because the variate generated is never used (opCall is called from the constructor without its return value being used, and at the end of that opCall _z is set to a sane value). However, this should arguably be fixed, since it's technically undefined behaviour.

@WebDrake
Copy link
Contributor Author

Further note: my own benchmarking of this suggests that performance actually improves slightly if _z is removed entirely, and L153 is replaced by

auto z = data[index];

Happy to submit a patch with that solution if it would be acceptable.

@WebDrake
Copy link
Contributor Author

Further note: my own benchmarking of this suggests that performance actually improves slightly if _z is removed entirely

... with a side benefit being that opCall no longer needs to be called from the constructor.

@9il
Copy link
Member

9il commented Dec 14, 2016

Presumably the intention/expectation is that _z should be initialized to data[index] in the constructor?

_z is not data[index] it is data[previous_index]

@9il
Copy link
Member

9il commented Dec 14, 2016

Further note: my own benchmarking of this suggests that performance actually improves slightly if _z is removed entirely, and L153 is replaced by

Please check checksums

@9il
Copy link
Member

9il commented Dec 14, 2016

The only reason to init _z is CTFE-able Mt, will init it with zero

@9il
Copy link
Member

9il commented Dec 14, 2016

Done 5a5e66e

@9il 9il closed this as completed Dec 14, 2016
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

No branches or pull requests

2 participants