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

Subject: Re: [boost] [Review] UUID library (mini-)review starts today, November 23rd
From: Andy Tompkins (atompkins_at_[hidden])
Date: 2008-11-27 20:41:56

On Thu, 27 Nov 2008 07:09:01 +1000, Vladimir.Batov_at_[hidden] 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