Fix Issue 9607 - std.random.randomShuffle() and partialShuffle() don't work with Xorshift. #1362
Conversation
don't work with Xorshift. This is an instance of Issue 2803, a clash between a template parameter and a default argument. I've used the workaround proposed in that issue thread: http://d.puremagic.com/issues/show_bug.cgi?id=2803#c1 Tests have been included to ensure that these functions work with all possible RNG types.
| @@ -112,6 +112,12 @@ version(unittest) import std.typetuple; | |||
| email: m-mat @ math.sci.hiroshima-u.ac.jp (remove space) | |||
| */ | |||
|
|
|||
| version(unittest) | |||
| { | |||
| alias TypeTuple!(MinstdRand0, MinstdRand, Mt19937, Xorshift32, Xorshift64, Xorshift96, | |||
There was a problem hiding this comment.
You should move this inside the unittest block where it's used, or it might conflict with user symbols when they compile with -unittest.
There was a problem hiding this comment.
The idea is that this TypeTuple be available to all unittest blocks so that they (and any dependent modules) can all do similar foreach() runs over all the available set of RNGs. I have a further patch in the pipeline which will extend that kind of testing to all std.random unittests, but I've withheld it for now as it's probably a good idea to wait until after pull request #1357 lands.
I could move the TypeTuple inside the randomShuffle unittest block for now and move it out again when the later patch is submitted -- your call. :-)
There was a problem hiding this comment.
Alternatively, would it be better to make it a global symbol that's not dependent on compiling with -unittest?
There was a problem hiding this comment.
In that case make it global but also add a ddoc comment (and I recommend using the syntax alias PseudoRngTypes = TypeTuple!(...)).
There was a problem hiding this comment.
In any case, even Inside a version(unittest) block, please make it package or private, or you could polute user unittest code.
There was a problem hiding this comment.
One possible reason to leave it inside a version(unittest) block -- it avoids having to import std.typetuple for non-unittest builds.
There was a problem hiding this comment.
unittest-based imports are error-prone. There was a problem in Phobos recently where an import was inside of a version(unittest) block, but because Phobos is never tested without the -unittest switch nobody caught a bug where compiling a user project which imported the module and where the user didn't have the -unittest switch caused the compilation to fail.
There was a problem hiding this comment.
OK, I'll make std.typetuple a regular import and remove the version(unittest) block around PseudoRngTypes. Would be interested to have a link to the bug you mention.
There was a problem hiding this comment.
Ah, I've mis-remembered what exactly it was about. Here's the bug:
http://d.puremagic.com/issues/show_bug.cgi?id=9269
Essentially someone added a version(unittest) import into a module which had a pragma(lib) call for the linker, leading to cases where the user could compile the code if they didn't use -unittest, but it would fail compilation if the user used -unittest but didn't have curl installed.
There was a problem hiding this comment.
Well, in that case I will keep the version(unittest) blocks, using the package alias ... formulation to make sure that it never clashes with anything outside std.random and documenting it with a comment rather than a ddoc section. Unless you specifically want me to do something else, in which case ... I'll do whatever you ask for. :-)
|
Will this do? Or would you still prefer the |
|
.... ping? :-) |
|
My testing demon is striking again. Anyway, re-ping? :-) |
|
Yeah it's good, let's just wait for the tester to pass. |
|
... ping? Seems an opportune time after #1403 got pulled :-) |
|
Ping again? It's now 3 weeks since the final version was uploaded, and it's now simply stuck on low priority in the autotester. |
|
I'm considering splashing out on the machine that goes 'ping' :-) |
Fix Issue 9607 - std.random.randomShuffle() and partialShuffle() don't work with Xorshift.
ghost
merged commit
|
Yeah. Btw, if you want to force a re-test usually what you do is just amend your last commit so it ends up having a new hash, then force push and the autotester will run from scratch. |
|
Thanks Andrej :-) I considered doing that, but TBH it seems ill-mannered to jump the queue. Anyway -- thank you again for the merge! |
|
Well unfortunately there are no 'pull went green' notifications, otherwise I'd pull this sooner. |
This is an instance of Issue 2803, a clash between a template parameter and a default argument. With no general solution on the horizon for that bug, I've used the workaround proposed in that issue thread:
http://d.puremagic.com/issues/show_bug.cgi?id=2803#c1
Tests have been included to ensure that these functions work with all possible RNG types. If this style of testing is considered useful I propose extending it to all functions that make use of RNGs.