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

Subject: Re: [boost] [Review] UUID library (mini-)review starts today, November 23rd
From: Vladimir.Batov_at_[hidden]
Date: 2008-11-26 16:09:01

*** What is your evaluation of the design?

Unfortunately I have serious reservations about the interface. Like,

1. "uuids" namespace seems unnecessary and burdensome. The generator 
classes 
can be introduced inside boost::uuid.
2. handling "char const*",  "wchar_t const*", std::basic_string-based 
arguments should be templated/generalized.
3. Using a separate "generator" class to create a new UUID seems 
unorthodox 
as I feel the standard way of calling a constructor would be more inline 
with the standard C++ practices. Like

    boost::uuids::basic_uuid_generator<boost::mt19937> gen2;
    boost::uuid id(gen2);

instead of

    boost::uuids::basic_uuid_generator<boost::mt19937> gen2;
    boost::uuids::uuid u2 = gen2();

That way

    boost::uuid id; // Works as Linux uuid_generate();
    boost::uuid id(gen2); // Works as Linux uuid_generate_random();

4. The default uuid() creating a "null" UUID is too subtle, wide open for 
misinterpretation and in fact counter-intuitive. For example, my (and many 

others that I know) interpretation of that constructor is like Linux 
uuid_generate() was called behind it. The creation of an invalid/null UUID 

needs to be explicitly stated with something like

    boost::uuid id = boost::uuid::null();
    boost::uuid id = boost::uuid::invalid();

then "is_null()" method would be somewhat expected/justified.

5. I'd expect to be able to use "if (!uuid) then..." rather than "if 
(uuid.is_null()) then...".
6. to_wstring() does not seem to belong to the class. I understand it is 
conveniend but I fdo not feel that is core UUID functionality and, 
therefore, should (and can) be handled by other means.

*** What is your evaluation of the implementation?

I am OK with the implementation with a few comments.

1. Unfortunately, it does not seem to provide time-based construction. I 
read explanations why not but the usefulness of such a simple concept very 

much depends on its completeness.
2. I was surprised to see NULL used and postincrements.
3. A lot of raw numbers around the code.
4. I am very suspicious of using std::streams as from what I know they are 

not exactly fast.

*** What is your evaluation of the documentation?

Well, in all honesty I do not feel like the uuid.html file supplied with 
the 
package (uuid_v13.zip) can be considered as adequate documentation. 
Explanations seem scetchy. Links seem broken (like "see operators.hpp" 
points to a non-existing "operators.htm").

*** What is your evaluation of the potential usefulness of the library?

I certainly think such a facility needed.

*** Did you try to use the library?  With what compiler?  Did you have any 

problems?

Tried with MSVC8 with results similar to already reported. Iseems like at 
least some of the warnings (like "warning C4244: '=' : conversion from 
'int' 
to 'char', possible loss of data") need to be addressed.

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

I spent a couple of hours with it.

*** Are you knowledgeable about the problem domain?

Well, I consider myself a "knowledgeable user". I certainly will not be 
able 
to claim to be intimately familiar with the generation algorithms.

*** Conclusion:

The UUID concept is most certainly very useful and such a facility is an 
essential gadget in everyone's toolbox. However, there are many 
implementations around (OpenLDAP, Linux, Windows to name a few). For 
boost::uuid to be accepted and used instead other alternatives it needs to 

be as complete as its competitors and to stand out -- to be superior in 
interface, convenience or something. In a couple of hours I spent with 
boost::uuid I unfortunately did not manage to get that impression.
Therefore, I am reluctantly voting against its acceptance in its present 
form.

Thanks,
Vladimir.