Review: std.uuid

[Jonathan M Davis]

On Saturday, June 16, 2012 13:27:54 Johannes Pfau wrote:
> Am Wed, 13 Jun 2012 18:57:51 -0700
> So I'd rather like to keep the name consistent with the RFC.

I understand. I don't think that there's a really a good solution. So, if you 
think that keeping it as-is is best, then we can do that.

> _toString is already nothrow, so yes, it's only idups fault ;-)
> Added links to http://d.puremagic.com/issues/show_bug.cgi?id=5700

Please just put it as a comment above the try-catch rather than putting it in 
the ddoc. No one using the function needs to know that there's a workaround 
for nothrow. They just need to know that it's nothrow. It's the Phobos 
maintainers that need to know that there's a workaround. It seems fairly 
common to do something like

//@@@BUG@@@ workaround for bugzilla 5700

so that it's nicely greppable.

> > * parseUUID should be clear on whether it takes both lowercase and
> > uppercase hex digits, and I'd argue that it should definitely accept
> > both. [0-9a-f] implies that it only accepts lowercase hex digits.
> 
> OK

Make sure that you have tests which test UUIDs with both uppercase and 
lowercase letters. I only see tests with lowercase letters. You seem to have 
updated an example with some uppercase letters but not any tests from what I 
can see. Speaking of which, please make sure that you have unittest blocks 
with your examples in them to make sure that they compile. Personally, I 
always put

//Verify Example.
unittest
{
    //example code...
}

after a function with an example and make sure that the example and the code 
in the unittest block match exactly (save for indentation, since any 
indentation in the example in the ddoc ends up in the final ddoc, and we do 
want the indentation in the unittest block).

> > * rndGen returns a Random (which is already aliased to Mt19937), and
> > already initializes it with an unpredictable seed. So, why not just
> > us rndGen in randomUUID? It becomes
> > 
> > return randomUUID(rndGen());
> > 
> > It's much shorter and just as good as far as I can tell.
> 
> Good catch. We'd have to change rndGen to use the new seed function
> though. And the documentation for Random says: "You may want to use it
> if [...] you don't care for the minutiae of the method being used"
> 
> I'm not sure, do we care about the method? We need something which
> generates 'good' random numbers, I don't think a LCG would be
> acceptable. But rndGen could default to something like that on embedded
> systems?
> 
> Then again, I know nothing about random number generators, so someone
> please tell me: Do we need  Mersenne twister for random UUIDS or could
> we use any RNG?

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

Yours
-----------
@trusted UUID randomUUID()()
{
    static Mt19937 randomGen;
    static bool seeded = false;
    if(!seeded)
    {
        randomGen.seed(map!((a) => unpredictableSeed)(repeat(0)));
        seeded = true;
    }
    return randomUUID(randomGen);
}
-----------

vs

-----------
alias Mt19937 Random;

@property ref Random rndGen()
{
    static Random result;
    static bool initialized;
    if (!initialized)
    {
        result = Random(unpredictableSeed);
        initialized = true;
    }
    return result;
}
-----------

Really, the only differences are the fact that you're passing the Random into 
randomUUID and returning that rather than returing the Random, and you're 
using a map to do the seed for reasons that completely escape me. In fact, I'm 
not sure how that code even compiles. It creates an infinite range whose every 
value is zero and then maps all of those zeroes to unpredictableSeed. So, 
you've created an infinite range of unpredictableSeed results. But Mt19937's 
seed takes a single uint, not a range. So, I'm baffled as to how that line even 
compiles.

In any case, it looks like they're both using Random, and they're both seeding 
it with an unpredictableSeed. It's just that in noe case, it's using Random's 
constructor to do it, while in the other, seed is being called explicitly 
(thereby _re_seeding the Random).

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.

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

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

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

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.

- Jonathan M Davis