Re: [Boost-users] [Boost-announce] [Review] UUID library (mini-)review starts today, November 23rd

Subject: Re: [Boost-users] [Boost-announce] [Review] UUID library (mini-)review starts today, November 23rd
From: Daryle Walker (darylew_at_[hidden])
Date: 2008-12-09 03:34:01

[Sorry for the lateness.  Haven't looked at any other reviews yet.]

On Nov 23, 2008, at 7:13 PM, Hartmut Kaiser wrote:

> The mini-review of Andy Tompkins UUID library starts today,  
> November 23rd
> 2008, and will end on November 30th.
> I really hope to see your vote and your participation in the  
> discussions on
> the Boost mailing lists!
>
> The library can be downloaded from the vault here (it's the file
> uuid_v13.zip in the topmost directory): http://tinyurl.com/6xx28b
>
> The initial review of the UUID library ended with a provisional  
> acceptance
> (read here: http://article.gmane.org/gmane.comp.lib.boost.user/ 
> 27774/).
>
> This mini review is meant to allow for a final decision after  
> looking at the
> changed parts of the library. Here is a list of things fixed and  
> changed:
>
> - fixed the licensing issues revealed by the initial review
> - fixed the security vulnerability revealed by the initial review
> - renamed to uuid, moved all classes, functions, etc. to namespace
> boost::uuids
> - implemented sha1 hash function (thus no license problem)

Others, including myself, are working on encoding libraries.  If this  
library, and an encoding library are added, we could change the  
implementation to use the new encoding library.

> - new class basic_uuid_generator to create random-based uuids.  The  
> random
> number generator is no longer hard coded.  It can use any random  
> number
> generator, default is boost::mt19937
> - implemented a good seed function for random number generators
> - all functions are now reentrant, all classes are as thread safe  
> as an int
> and the library is no longer dependent on Boost.Thread
>
> ---------------------------------------------------
>
> Please always state in your review, whether you think the library  
> should be
> accepted as a Boost library!

Yes, please accept it.

>
> Additionally please consider giving feedback on the following general
> topics:
>
> - What is your evaluation of the design?

Why is there a container interface for inspecting the value's  
octets?  For a similar problem in my code (the MD5 stuff in the  
Sandbox SVN), I defined a copying-out member function template that  
takes an output iterator and returns the updated version of same.   
You could do that, making sure to define some (compile-time) item  
indicating the length (16), to facilitate easier changes to internal  
storage.

Don't some operating systems provide UUID generation?  Could there be  
a way to add a creation function that uses the OS's code?

Is default construction useful besides when initialization can't be  
done in one step? Instead of an "is_null" member function, maybe  
(pseudo-)Boolean conversion (and "operator not") can be used.

I don't think the multitude of string conversion techniques are  
needed.  Keep the constructor with "char const *" for pseudo- 
literals.  Everything else, in either direction, could use  
"lexical_cast".

> - What is your evaluation of the implementation?

The "generator_iterator" substitute in "seed_rng.hpp" doesn't  
completely help.  It is supposed to improve on the version in "boost/ 
generator_iterator.hpp" in the area of end-iterator support.  I  
originally said more here, but I erased it because when I filed a  
ticket on this issue, I found one already (#2428) and put my notes  
there.

The implementation of the constructor that takes a byte-returning  
input-iterator shows why we need a double-bounded copy algorithm.   
(Now in new ticket #2578.)

The copy constructor, destructor, and copy-assignment operator don't  
do anything different than the automatically-defined versions would.   
So they could be omitted.

Maybe the "create_name_based" and "get_showbraces_index" member  
functions need to be in a new mandatory source file.  If so, you  
could move some common error strings there too.  BTW, why do  
"create" (and "create_name_based") need the length based in as a  
separate argument?  Just use "std::strlen".

The I/O function templates could be tightened up some.  (I noticed it  
since I work on I/O quite a bit.  It could be done later.)  The  
optimization would be more important if my dump-most-of-the-string- 
conversions-for-lexical_cast idea is used.

Should the serialization be done in a separate header?  BTW, how is a  
custom primitive type handled?  Is it just a byte-level save/load of  
memory?  (What if the type isn't POD, like this one?)

> - What is your evaluation of the documentation?

It's adequate, but maybe detailed Doxygen information on each item  
should be added.

> - What is your evaluation of the potential usefulness of the library?

It can be useful for (temporary) IDs.  We have to make sure that the  
type, especially its object representation, doesn't get heavy (i.e.  
stay quasi-POD).

> - Did you try to use the library?  With what compiler?  Did you  
> have any
> problems?

I used it on Mac OS X 10.4 (PowerPC) with XCode.  All the tests, and  
Paul Bristow's example, seemed to work.

> - How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?

Two days of reading and trying out the code.  Not too much of the  
theory, though.

> - Are you knowledgeable about the problem domain?

Not really.

-- 
Daryle Walker
Mac, Internet, and Video Game Junkie
darylew AT hotmail DOT com