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: Andy Tompkins (atompkins_at_[hidden])
Date: 2008-12-09 12:17:09

On Tue, 9 Dec 2008 03:34:01 -0500, "Daryle Walker"
<darylew_at_[hidden]> said:
> [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.

Yes, I would gladly change the uuid library 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.

This was a requested feature.

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

Yes, I will write generators that use OS 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.

The default constructor is unintuitive.  I will likely remove it.  I
will 
add a 'boolean' conversion and possibility operator!.

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

I agree, lexical_cast is good.  I will remove the to_..._string
functions.  
The constructor that takes a string will likely move to a generator.

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

Thanks.  Ticket #2428 was added in response to the uuid library by
someone else.

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

Fair.

> 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 create_name_based function will be moved to a generator.  I added
the 
length so that one could use it with any given object.  But this is not 
what people want, so yes, it can just use std::strlen (as well as have 
overloads for std::basic_string<>).

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

Yes, the i/o functions need improving.  I will likely need some help
when 
the time comes.

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

Yes, it is a byte-level save/load.  My reason was so that exactly 128
bytes 
are saved/loaded.

> > - What is your evaluation of the documentation?
>
> It's adequate, but maybe detailed Doxygen information on each item
> should be added.

Agreed.

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

Agreed.

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

Thanks,
Andy Tompkins