On Thu, 27 Nov 2008 07:09:01 +1000, Vladimir.Batov@wrsa.com.au said:

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

I introduced namespace boost::uuids in response to http://www.boost.org/development/requirements.html#Naming_consistency The uuid library is small and could live in namespace boost. I felt it to be good practice to introduce namespace boost::uuids. If there is strong opinion to move it to namespace boost, I will move it.

2. handling "char const*", "wchar_t const*", std::basic_string-based arguments should be templated/generalized.

Hmm, I did use template member functions for std::basic_string<>. Are you thinking of the following constructor for char/wchar_t? class uuid { public: template <typename ch> uuid(ch const*const str); };

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

I have never though of this. I would then likely have something like: // null uuid, act like Linux uuid_clear() boost::uuids::uuid u1; // time-based uuid, act like Linux uuid_generate_time() boost::uuids::uuid_generator_time gen2; boost::uuids::uuid u2(gen2); // random-based uuid, act like Linux uuid_generate_random() boost::uuids::uuid_generator_random gen4; boost::uuids::uuid u4(gen4); // name-based uuid boost::uuids::uuid_generator_name gen3; boost::uuids::uuid u3(gen3);

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.

I would not want the default constructor to generate a uuid. I don't want users to pay for that if they don't want it. Some use cases will not require generating uuids, only using them. I don't mind adding boost::uuids::uuid u = boost::uuids::uuid::null() that acts like Linux uuid_clear().

5. I'd expect to be able to use "if (!uuid) then..." rather than "if (uuid.is_null()) then...".

I can add operator!() as syntactic sugar if this list wants it.

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.

I have no strong feeling to leave it in or take it out. I agree that the to_string() functions are convenience functions that 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.

I will seriously consider providing a time-based generator.

2. I was surprised to see NULL used and postincrements.

I can easily take out NULL and post-increments. Would you point out a few post-increments that you see?

3. A lot of raw numbers around the code.

Hmm, the ones I'm seeing are in the document, http://www.itu.int/ITU-T/studygroups/com17/oid/X.667-E.pdf. What ones would you like to see removed? I can make constants for them, or just document them better.

4. I am very suspicious of using std::streams as from what I know they are not exactly fast.

I assume you mean in the constructors and the to_string functions. Can you suggest an alternative?

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

I have had some good suggestion from this list on ways to improve the documentation and I will do these. I will fix the operators.hpp link. The rest of the links seem fine to me. Were there others that you found?

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

I will address these.

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

Thanks, Andy Tompkins