[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