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

On Mon, 1 Dec 2008 20:05:58 -0600, "Christian Holmquist"
<c.holmquist <at> gmail.com> said:
> 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.

Good point.  I wonder if they should both be generators.  A generator
that takes a string or an iterator range for a string.  And a different
generator that takes an array of bytes or an iterator range for bytes.

> 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?

No reason to use one over the other, just preference.  It has been
mentioned before and I will likely remove the to_..._string functions.

> 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?

It could easily be made static.  It will _always_ return 16.

> 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?

Maybe.  Maybe uninitialized, nil.  I don't think it matters too
much because I don't think there is a name that everyone will think
is obvious.

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

I agree this should be a generator.

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

Yes it should have random in its name.

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

Yes, I like this.

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

This could easily be fixed by separating functionality out into
different 
header files.  I will do this.

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

Yes, I will do this.

> - 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 not keen on this.  The fact that boost::uuid is implemented with 
boost::array is just an implementation detail.  That may change.  I 
want the public interface to remain the same.

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

I agree, separating io, generation, ... should be done.  I will do this.

> Regards, Christian

Thanks,
Andy Tompkins