Review: std.uuid

[Johannes Pfau]

Am Sat, 16 Jun 2012 16:57:25 -0700
schrieb Jonathan M Davis <jmdavisProg at gmx.com>:

> Umm. My point was that as far as I can tell, what you're doing is
> _identical_ to what rndGen is doing.
> 

Yes right now it's exactly the same. But there's no guarantee that the
'Random' alias (and therefore rndGen) will always use Mt19997.

The docs explicitly say:
--------------
The "default", "favorite", "suggested" random number generator type on
the current platform. It is an alias for one of the previously-defined
generators. You may want to use it if (1) you need to generate some
nice random numbers, and (2) you don't care for the minutiae of the
method being used.
--------------

so it could be an alias to a 'worse' RNG on some platforms and I'm not
sure if that's OK.

I don't mind changing it to rndGen if someone can tell me that it's fine
to use 'worse' RNGs for UUIDs, but boost used MersenneTwister and I
don't want to change that unless I'm sure it won't cause problems.

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

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

> > > * 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)

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

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