Review: std.uuid

[Johannes Pfau]

Am Sun, 10 Jun 2012 13:02:16 +0400
schrieb Dmitry Olshansky <dmitry.olsh at gmail.com>:

> On 09.06.2012 21:30, Dmitry Olshansky wrote:
> > The review process stalled long enough, let's kick start it with a
> > small yet a valuable module that was there for quite some time.
> >
> 
> Ok, let's grill it a bit ;)
> 
>  From browsing the docs alone:
>   ParserException - too general name likely to collide with user
> code, call it UuidException instead
>   InsufficientInputException - maybe merge it with ParserException,
> just add small <reason> field
> 
> Why? - Typical user can't do much with this extra info. By the end of 
> day an illegal UUID string is an illegal UUID string.

OK. Is UUIDParserException too long? I'd like to keep 'parser' or
something similar in the name, as almost all UUID operations are
nothrow and 'UUIDException' sounds like a UUID may blow up at any time.

> 
> Add a constructor that takes variadic ubyte[] arr.../byte[] arr..
> that should just assert that argument has 16 bytes to hold UUID. (all
> these casts in docs ...)

Good idea!
But wouldn't it be better to use this(ubyte[16] uuid...) as described on
http://dlang.org/function.html (Typesafe Variadic Functions: For static
arrays)

Meh - we still can't get rid of the casts: DMD complains:
--------------
std/uuid.d(260): Error: template std.uuid.UUID.__ctor cannot deduce
template function from argument
types !()(int,int,int,int,int,int,int,int,int,int,int,int,int,int,int,int)
--------------
and AFAIK there's no integer suffix to specify a ubyte value, so we end
up with this:
UUID tmp =
    UUID(cast(ubyte)0,cast(ubyte)1,cast(ubyte)2,cast(ubyte)3,
    cast(ubyte)4,cast(ubyte)5,cast(ubyte)6,cast(ubyte)7,cast(ubyte)8,
    cast(ubyte)9,cast(ubyte)10,cast(ubyte)11,cast(ubyte)12,
    cast(ubyte)13,cast(ubyte)14,cast(ubyte)15);

> 
> isNil - can we follow the precedent and use 'empty' for "has no
> useful state" as does for instance std.regex.Regex and all of Range
> API.

OK

> 
> Variant - may be confusing with as I thought of std.Variant till the 
> bitter end :)

Do you know a better name? Both the wikipedia article and rfc4122 call
it variant though, so I'd like to keep that name. I could probably add
a Note: Do not confuse with std.variant.Variant warning.

> uuidVersion - sadly we can't fix it's name ;)

We could use '_version' or 'version_' but I think uuidVersion is the
better solution.

> 
> nilUUID should be a property that returns UUID.init or better an
> enum, no need to clutter object file with _TLS_ globals.

It already is an enum?
line 771: enum UUID nilUUID = UUID.init;

> parseUUID - like Walter pointed out, could use input range

See my response to Walter.

> BTW functions taking a namespace UUID can take const UUID.

OK. (Although the namespace uuids are passed by value, so that shouldn't
be necessary)

> extractRegex ---> uuidRegex what's easier from user standpoint?

OK, renamed to uuidRegex

> 
> Now the code:
> 
> line 587: swap surely could do better then this:
>              swapRanges(this.data[], rhs.data[]);
> e.g. swap it as 4 uints or 2/4 size_ts. Main reason for built-in swap
> is efficiency. As it is  now it won't surprise me if std.swap would
> be faster with its 3 bit-blits.

you mean std.algorithm.swap?

> 
> line 1011: overload with no args
> 	Creating Random generator on each call is unacceptable. It's
> better to either remove it or use thread-local _static_ RNG here.
> 
> 

You mean simply declaring randomGen static in line 1013? Then the seed
function would still be called every time. Or making randomGen a
private module level TLS variable and calling seed in the module
constructor?

What about the overload only taking a type? It also creates the RNG on
every call. To avoid this I could make randomGen static, but there's no
way to avoid calling seed every time.

Another possibility is to make uuid generators structs instead of
functions, but that complicates the API and the randomUUID overload
which takes a RNG variable should already be as fast as that.



BTW: Is it OK to update the code & docs while the review is running or
should I wait with those changes till the review is finished?