Am Sun, 17 Jun 2012 01:10:16 -0700 schrieb Jonathan M Davis <[email protected]>:
> I seriously question that it will _ever_ be anything worse then > Mt19997, but if you're that worried about it, maybe you should add a > static assert that Random is Mt19997? If you want to leave it as-is, > then you should probably at least put a comment in there as to why > you're using Mt19997 instead of just using rndGen, otherwise, someone > may come along and change it to use rndGen later. OK, I added the static assert and use rndGen now. I'll have to update the pull request so that the new seeding method is used in rndGen though. > > > So, I don't see what you're doing that's any different from just > > > doing > > > > > > return randomUUID(rndGen()); > > > > > > aside from the fact that you seem to have an impossible line of > > > code for seeding your random number generator. And trying to > > > compile that line on my computer actually fails (as I'd expect), > > > so I don't know how it's compiling on yours. > > > > Yes, that code requires a new overload for seed, see > > https://github.com/D-Programming-Language/phobos/pull/627 > > And yes, it's generating a infinite range of unpredictableSeed > > (Mt19937 uses 624 values from that range) > > > > I don't know anything about RNGs, but Andrew Talbot suggested in the > > thread called "Mersenne Twister Seeding and UUIDs" that we need this > > new, better seeding method. > > Ah, okay. Apparently I missed that pull request. I'll have to look it > over, particularly since I seem to be pretty much the only one > merging anything in of late (particularly since Andrei has been too > busy to do much with D lately). > > > > > I don't think that's a good idea. If the randomGen.front the is > > > > e.g. ubyte, casting it to uint will produce 3 bytes which are > > > > not set to a random value. That's not acceptable. > > > > > > > > ------------ > > > > @trusted UUID randomUUID(RNG)(ref RNG randomGen) > > > > if(isUniformRNG!(RNG) && > > > > > > > > isIntegral!(typeof(RNG.front)) && typeof(RNG.front).sizeof > > > > <= > > > > > > > > 16) { > > > > > > > > enum elemSize = typeof(RNG.front).sizeof; > > > > UUID u; > > > > foreach(size_t i; iota(0, 16, elemSize)) > > > > { > > > > > > > > randomGen.popFront(); > > > > immutable randomValue = randomGen.front; > > > > u.data[i .. i + elemSize] = > > > > > > > > *cast(ubyte[elemSize]*)&randomValue; } > > > > ------------ > > > > ? > > > > > > Looks good except for the fact that the typeof(RNG.front).sizeof > > > <= 16 is completely unnecessary. isIntgeral guarantees that the > > > type is a built-in integral type, the largest of which is 8 bytes > > > (and even if we added cent and ucent, it would still be only 16, > > > not greater than 16). > > > > I'll make it a static assert. It might not be necessary, but it at > > least documents that elemSize can't be > 16 and it's another safety > > measure. > > But it's _completely_ unnecessary. You've already guaranteed it with > isIntegral. I guess that you can leave it there if you want to, but > as far as I can see, it's pointless. > > > > > > * For your functions which can take range types, test with > > > > > filter!"true" so that you get a range which isn't an array or > > > > > string. > > > > > > > > Does take!(string, int) return a slice? Should have thought > > > > about that.... > > > > > > No. It doesn't, because hasSlicing!string is false. > > > > > > > I can't use filter (and map) though: > > > > std/algorithm.d(1141): Error: nested structs with constructors > > > > are not yet supported in CTFE (Bug 6419) std/uuid.d(1273): > > > > called from here: > > > > filter("00000000-0000-0000-0000-000000000000"d) > > > > > > filter!"true"("00000000-0000-0000-0000-000000000000") should > > > compile just fine. You then pass that to your range-based uuid > > > functions which accept ranges or dchar. What does CTFE have to do > > > with it? That would only be an issue if you were trying to use > > > filter in CTFE, and even if you have CTFE tests where you'd like > > > to do that and can't due to that bug, there are plenty of tests > > > that you could run which weren't CTFE. I think that one or both > > > of us is misunderstanding something here. > > > > I _do_ have CTFE tests. parseUUID and UUID.this(string) are > > guaranteed to work in CTFE as well. And if I have to use a custom > > Range for the CTFE tests anyway, why not use it for all tests? > > (This also allows me to explicitly test Forward and Input Ranges) > > If you have to create other ranges for your tests anyway, then that's > fine. But you seemed to be saying that filter didn't work at all, and > it should. > > > > > Thanks for your feedback! > > > > > > Also, I just noticed that it looks like you don't have any tests > > > which verify that empty is false when it's supposed to be. You > > > should probably add a few. > > > > There's one such test, but I can add some more. > > You probably don't need a lot, but it does need to be tested, and I > didn't see any tests for empty which were on a UUID which wasn't > supposed to be empty. > > > > And by the way, before this actually gets merged into Phobos (as > > > I'm guessing that it will be, since no way seems to particularly > > > dislike the module thus far), you should fix the ddoc to use LREF > > > and XREF when referencing symbols elsewhere in the module and > > > elsewhere in std respectively. > > > > OK, I replaced some $(D ) with XREF/LREF but MYREF1,2,3 probably > > have to stay. Those are needed for the table (at least it's done > > that way in std.algorithm) > > Stuff for the table is fine. It's primarily an issue of the ddoc > directly on functions which you've been using $(D ) with, since that > doesn't create links to anything. > > - Jonathan M Davis
