View mode: basic / threaded / horizontal-split · Log in · Help
June 09, 2012
Review: std.uuid
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.

std.uuid by Johannes Pfau is a rehash of it's C++ twin from well known 
Boost library.

The review cycle takes the usual 2 weeks starting today 9th June, ending 
at 23 June 00:00 UTC followed by a one week vote process ending at 30 June.

The module gives the following description:
---------------------
This is a port of boost.uuid from the boost project with some minor
additions and API changes for a more D-like API. A UUID, or Universally
unique identifier, is intended to uniquely identify information in a
distributed environment without significant central coordination. It
can be used to tag objects with very short lifetimes, or to reliably
identify very persistent objects across a network. UUIDs have many
applications. [...]
---------------------

Links & Info

Code: https://github.com/jpf91/phobos/blob/std.uuid/std/uuid.d
API-Docs: http://dl.dropbox.com/u/24218791/d/src/uuid.html

Additional note from Johannes:

The code and documentation for shaUUID has already been written,
but until phobos has support for SHA1, that can't be included. The code
is currently commented out in the source file (it's well tested
with some 3rd party SHA1 code), but the documentation for those
functions is included in the API-docs. I think those functions should
be reviewed as well, so that it's possible to add them to phobos with a
simple pull request at a later date.

-- 
Dmitry Olshansky
June 10, 2012
Re: Review: std.uuid
On 6/9/2012 10:30 AM, 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.

Great! My initial suggestion is that the parsing routines should accept input 
ranges, rather just arrays.
June 10, 2012
Re: Review: std.uuid
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.

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

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.

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

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

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

parseUUID - like Walter pointed out, could use input range

BTW functions taking a namespace UUID can take const UUID.

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

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.

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.


-- 
Dmitry Olshansky
June 10, 2012
std.uuid: Ranges as parser input
Am Sat, 09 Jun 2012 23:06:25 -0700
schrieb Walter Bright <newshound2@digitalmars.com>:

> On 6/9/2012 10:30 AM, 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.
> 
> Great! My initial suggestion is that the parsing routines should
> accept input ranges, rather just arrays.
> 

That could be done, but I currently keep the complete input string in
the ParserException. That wouldn't be possible if the parser has to
operate on an InputRange. It could work with a ForwardRange, but
converting the Range back to a string wouldn't be very efficient. This
would work best with some kind of BufferedRange/Stream concept.

I'm just wondering whether it really makes sense to use ranges in this
case. The text representation of UUIDs is usually exactly 36 characters
long (for the simple parser it must be exactly 36 characters), so when
using some kind of range interface and std.uuid it should be easy enough
to read the UUID string into a stack buffer, then pass it to std.uuid.

But if it's really desired, I can sure add an implementation using
ForwardRanges.
June 10, 2012
Re: Review: std.uuid
On Saturday, 9 June 2012 at 17:31:02 UTC, 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.

When looking at the associated RFC I noticed that a UUID can 
contain e.g. timestap, clock, node fields. What about providing 
properties that could return these fields?

It would also be nice to have an index in the top of the page 
like std.algorithm has.

Jonas
June 10, 2012
Re: Review: std.uuid
Am Sun, 10 Jun 2012 12:03:15 +0200
schrieb "Jonas Drewsen" <jdrewsen@nospam.com>:

> When looking at the associated RFC I noticed that a UUID can 
> contain e.g. timestap, clock, node fields. What about providing 
> properties that could return these fields?

Why would you need those? A UUID only needs to be unique, which is
guaranteed by those node, timestamp and clock fields. But the exact
value of node, timestamp, clock is never important, as long as it's
unique. std.uuid can't create version 1&2 uuids so we never have to set
these fields either.

> It would also be nice to have an index in the top of the page 
> like std.algorithm has.

OK, I'll have a look at that.
June 10, 2012
Re: Review: std.uuid
Am Sun, 10 Jun 2012 13:02:16 +0400
schrieb Dmitry Olshansky <dmitry.olsh@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?
June 10, 2012
Re: std.uuid: Ranges as parser input
On 6/10/2012 2:05 AM, Johannes Pfau wrote:
> Am Sat, 09 Jun 2012 23:06:25 -0700
>> Great! My initial suggestion is that the parsing routines should
>> accept input ranges, rather just arrays.
>>
>
> That could be done, but I currently keep the complete input string in
> the ParserException. That wouldn't be possible if the parser has to
> operate on an InputRange. It could work with a ForwardRange, but
> converting the Range back to a string wouldn't be very efficient. This
> would work best with some kind of BufferedRange/Stream concept.
>
> I'm just wondering whether it really makes sense to use ranges in this
> case. The text representation of UUIDs is usually exactly 36 characters
> long (for the simple parser it must be exactly 36 characters), so when
> using some kind of range interface and std.uuid it should be easy enough
> to read the UUID string into a stack buffer, then pass it to std.uuid.
>
> But if it's really desired, I can sure add an implementation using
> ForwardRanges.

I can see it wouldn't make sense for the simple parser, but for the more general 
one it may. You can also specialize the more general parser for arrays, so there 
won't be the overhead of making a copy.
June 10, 2012
Re: Review: std.uuid
On 10.06.2012 15:22, Johannes Pfau wrote:
> Am Sun, 10 Jun 2012 13:02:16 +0400
> schrieb Dmitry Olshansky<dmitry.olsh@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
June 10, 2012
std.uuid: Update 1
Am Sat, 09 Jun 2012 21:30:57 +0400
schrieb Dmitry Olshansky <dmitry.olsh@gmail.com>:
> Code: https://github.com/jpf91/phobos/blob/std.uuid/std/uuid.d
> API-Docs: http://dl.dropbox.com/u/24218791/d/src/uuid.html

I pushed these changes suggested by Dmitry Olshansky and Jonas Drewsen:

* Add documentation table
* Rename UUID.isNil --> UUID.empty
* Merge ParserException and IsufficientInputException into
 UUIDParserException
* Add note about std.variant.Variant
* Rewrite example to avoid cast
* Add non-working(commented out) variadic constructor
* Rename extractRegex --> uuidRegex
* Use std.algorithm.swap instead of swapRanges

Both the source file and the documentation have been updated.
« First   ‹ Prev
1 2 3 4 5
Top | Discussion index | About this forum | D home