Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix Issue 9607 - std.random.randomShuffle() and partialShuffle() don't work with Xorshift. #1362

Merged
2 commits merged into from Jul 15, 2013

Conversation

Projects
None yet
2 participants
Contributor

WebDrake commented Jun 22, 2013

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.

Fix Issue 9607 - std.random.randomShuffle() and partialShuffle()
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.

@ghost Unknown and 2 others commented on an outdated diff Jun 22, 2013

@@ -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,
@ghost

ghost Jun 22, 2013

You should move this inside the unittest block where it's used, or it might conflict with user symbols when they compile with -unittest.

@WebDrake

WebDrake Jun 22, 2013

Contributor

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. :-)

@WebDrake

WebDrake Jun 22, 2013

Contributor

Alternatively, would it be better to make it a global symbol that's not dependent on compiling with -unittest?

@ghost

ghost Jun 22, 2013

In that case make it global but also add a ddoc comment (and I recommend using the syntax alias PseudoRngTypes = TypeTuple!(...)).

@monarchdodra

monarchdodra Jun 22, 2013

Collaborator

In any case, even Inside a version(unittest) block, please make it package or private, or you could polute user unittest code.

@monarchdodra

monarchdodra Jun 22, 2013

Collaborator

EDIT: Unless we want to make it a fully public and documented symbol, but I'm not sure I see the point outside of testing purposes, so I don't think it would be useful for the end user...

@WebDrake

WebDrake Jun 22, 2013

Contributor

Well, it occurred to me that if I was writing code that consumed random numbers, I'd like to have that TypeTuple available for testing purposes -- but it's hardly difficult for a user to retype. So I think I'll make it a private symbol, and propose it as something to have publicly in std.random2 or whatever we wind up calling it. Patch to follow shortly.

@WebDrake

WebDrake Jun 22, 2013

Contributor

One possible reason to leave it inside a version(unittest) block -- it avoids having to import std.typetuple for non-unittest builds.

@ghost

ghost Jun 22, 2013

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.

@WebDrake

WebDrake Jun 22, 2013

Contributor

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.

@ghost

ghost Jun 22, 2013

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.

@WebDrake

WebDrake Jun 22, 2013

Contributor

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. :-)

Contributor

WebDrake commented Jun 26, 2013

Will this do? Or would you still prefer the version(unittest) brackets to be removed?

Contributor

WebDrake commented Jul 2, 2013

.... ping? :-)

Contributor

WebDrake commented Jul 5, 2013

My testing demon is striking again. Anyway, re-ping? :-)

@ghost

ghost commented Jul 5, 2013

Yeah it's good, let's just wait for the tester to pass.

Contributor

WebDrake commented Jul 10, 2013

... ping? Seems an opportune time after #1403 got pulled :-)

Contributor

WebDrake commented Jul 13, 2013

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.

Contributor

WebDrake commented Jul 15, 2013

I'm considering splashing out on the machine that goes 'ping' :-)

ghost pushed a commit that referenced this pull request Jul 15, 2013

Merge pull request #1362 from WebDrake/randomshuffle
Fix Issue 9607 - std.random.randomShuffle() and partialShuffle() don't work with Xorshift.

@ghost ghost merged commit 443b54e into dlang:master Jul 15, 2013

1 check was pending

default Pass: 4, Pending: 6
Details
@ghost

ghost commented Jul 15, 2013

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.

Contributor

WebDrake commented Jul 15, 2013

Thanks Andrej :-) I considered doing that, but TBH it seems ill-mannered to jump the queue. Anyway -- thank you again for the merge!

@WebDrake WebDrake deleted the WebDrake:randomshuffle branch Jul 15, 2013

@ghost

ghost commented Jul 15, 2013

Well unfortunately there are no 'pull went green' notifications, otherwise I'd pull this sooner.

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment