Review: std.uuid

Dmitry Olshansky dmitry.olsh at gmail.com
Sun Jun 10 09:37:34 PDT 2012


On 10.06.2012 15:22, Johannes Pfau wrote:
> 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?

It's fine as long as you keep as informed of updates ;)

-- 
Dmitry Olshansky


More information about the Digitalmars-d mailing list