On Tue, 9 Dec 2008 03:34:01 -0500, "Daryle Walker" <darylew@hotmail.com> 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