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

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.

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

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.

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

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.

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?

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.

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

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.