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