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