Review: std.uuid

Johannes Pfau nospam at example.com
Sun Jun 17 01:38:50 PDT 2012


Am Sun, 17 Jun 2012 01:10:16 -0700
schrieb Jonathan M Davis <jmdavisProg at gmx.com>:

> 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




More information about the Digitalmars-d mailing list