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

Subject: Re: [Boost-users] [Review] UUID library (mini-)review starts today, November 23rd
From: Christian Holmquist (c.holmquist_at_[hidden])
Date: 2008-12-01 21:05:58

Hi,
Maybe too late for a mini-review, but here's some unordered remarks.

>From uuid.hpp

template <typename ch, typename char_traits, typename alloc>
explicit uuid(std::basic_string<ch, char_traits, alloc> const& str);

Creating a stringstream and parsing from a user-supplied string seems
dangerous to me.
It could be convenient to some, but I wouldn't go for an API where

std::string str = ...;
uuid id1(str);
uuid id2(str.begin(), std.end());

Doesn't do the same thing.

std::string to_string() const;
std::wstring to_wstring() const;
template <typename ch, typename char_traits, typename alloc>
std::basic_string<ch, char_traits, alloc> to_basic_string() const;

Any reason to use to_string instead of std::basic_ostream operators instead?

size_type size()
The underlying container is boost::array, shouldn't this function be made
static?
Or is the size of the uuid implementation defined and the user shouldn't
count on it's length?

is_null()
What does this mean?
Ah, ok, from the docs I see it's a magic uuid (all zero) that is_null. Maybe
is_zero() would be more clear?

static uuid create(uuid const& namespace_uuid, char const* name, int
name_length);
Can this be a generator instead?

std::string name="www.widgets.com";
name_based_generator gen(name.begin(), name.end());
uuid id = gen();

Shouldn't basic_uuid_generator be named random_uuid_generator?
It doesn't seem more basic than any other generator.

- What is your evaluation of the design?
Id like to see more separation between the generators and the core uuid
class. IMO generating
functionality should not be part of the uuid class at all, but separated
into own headers with
well documented behaviour.
I'd like to se
random_generator<class RandomGen>
parsing_generator<class CharIterator>
native_generator<..> (wrapper around os API)

- What is your evaluation of the implementation?
It seems fine, but it's not organized IMO. As a user I don't want to pay
compile time
for features I do not use (from uuid.hpp):
#include <boost/operators.hpp>
#include <boost/io/ios_state.hpp>
#include <boost/random/uniform_int.hpp>
#include <boost/random/variate_generator.hpp>
#include <boost/random/mersenne_twister.hpp>
#include <boost/uuid/seed_rng.hpp>
#include <sstream>
#include <iosfwd>
#include <locale>

- What is your evaluation of the documentation?
I'd like to see more information about how to create uuids and if the
library can help me
doing that. Also what I can expect from the different generators (speed vs
uniqueness).

- What is your evaluation of the potential usefulness of the library?
Very useful.

- Did you try to use the library?  With what compiler?  Did you have any
problems?
Didn't try.

- How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?
Reading the documentation and headers. About 2 hours.

- Are you knowledgeable about the problem domain?
Yes, as a user of them (not generation).

> Please always state in your review, whether you think the library should
be
> accepted as a Boost library!
I vote no for now.
I think most of the functionality needed is implemented (except the
os_generator),
but needs refactoring.

Maybe this is out of the question to most, but is the uuid class needed at
all?
I'd be happy to see the following working (cause then I can use our own uuid
class with the algorithms provided by the library).

#include <boost/uuid/random_generator.hpp>
#include <boost/uuid/support/array.hpp>
#include <boost/uuid/io.hpp>

typedef boost::array<char, 16> id_type;
boost::uuid::random_generator<id_type, boost::mt19937> uuid_gen(...);
my_id id = uuid_gen();
std::cout << boost::uuid::format(id) <<std::endl;

I admit the last part is from the top of my head right now, there's probably
flaws with it =)
But separating io, generation and container should IMO be done either way.

Regards,
Christian