[Review] UUID library (mini-)review starts today, November 23rd
Hi all, The mini-review of Andy Tompkins UUID library starts today, November 23rd 2008, and will end on November 30th. I really hope to see your vote and your participation in the discussions on the Boost mailing lists! The library can be downloaded from the vault here (it's the file uuid_v13.zip in the topmost directory): http://tinyurl.com/6xx28b The initial review of the UUID library ended with a provisional acceptance (read here: http://article.gmane.org/gmane.comp.lib.boost.user/27774/). This mini review is meant to allow for a final decision after looking at the changed parts of the library. Here is a list of things fixed and changed: - fixed the licensing issues revealed by the initial review - fixed the security vulnerability revealed by the initial review - renamed to uuid, moved all classes, functions, etc. to namespace boost::uuids - implemented sha1 hash function (thus no license problem) - new class basic_uuid_generator to create random-based uuids. The random number generator is no longer hard coded. It can use any random number generator, default is boost::mt19937 - implemented a good seed function for random number generators - all functions are now reentrant, all classes are as thread safe as an int and the library is no longer dependent on Boost.Thread --------------------------------------------------- Please always state in your review, whether you think the library should be accepted as a Boost library! Additionally please consider giving feedback on the following general topics: - What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With what compiler? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain? Regards Hartmut Review Manager
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Hartmut Kaiser Sent: 24 November 2008 00:14 To: boost@lists.boost.org; boost-users@lists.boost.org; boost- announce@lists.boost.org Subject: [boost] [Review] UUID library (mini-)review starts today, November 23rd
The mini-review of Andy Tompkins UUID library starts today, November 23rd 2008, and will end on November 30th. I really hope to see your vote and your participation in the discussions on the Boost mailing lists!
The library can be downloaded from the vault here (it's the file uuid_v13.zip in the topmost directory): http://tinyurl.com/6xx28b
The initial review of the UUID library ended with a provisional acceptance (read here: http://article.gmane.org/gmane.comp.lib.boost.user/27774/).
Re-reading the docs, I can't see any reasons against acceptance. Nits: I note that the docs uuid.html copyright date is still only 2006. And it does not list/link all the tests. Mis spelling of 'hexidecimal' The external representation of a uuid is a string of hexidecimal digits Note: boost::uuids::uuid::size() always returnes 16. Mispelled 'returnes' Is there a reason why create does not also take a std::string (with default length .size() as parameter?) // Static functions static uuid create(uuid const& namespace_uuid, char const* name, int name_length); I assumed it would exist and was surprised when it didn't. (Should the name_length have a default value? C-string size - 1?) I also believe that a really basic example would be useful. This helps novices. A little demo I knocked up quickly attached. (MSVC 8 Sp1) It reveals that the hated 4996 warnings are triggered (at default MS warning level 3). I think these need to be suppressed with push'n'popping. Also I got a faceful of these at warning level 4. Again they should be suppressed. 1>I:\boost_1_37_0\boost/random/detail/pass_through_engine.hpp(49) : warning C4512: 'boost::random::detail::pass_through_engine<UniformRandomNumberGenerator>' : assignment operator could not be generated 1>H:\uuid\boost/uuid/uuid.hpp(364) : warning C4244: '=' : conversion from 'int' to 'char', possible loss of data Should be silenced using a static_cast? c = static_cast<ch>(is.peek()); But overall this seems 'OK to ship'. HTH Paul --- Paul A. Bristow Prizet Farmhouse Kendal, UK LA8 8AB +44 1539 561830, mobile +44 7714330204 pbristow@hetp.u-net.com
On Mon, 24 Nov 2008 16:59:10 -0000, "Paul A. Bristow" <pbristow@hetp.u-net.com> said: < snip >
Re-reading the docs, I can't see any reasons against acceptance.
Nits:
I note that the docs uuid.html copyright date is still only 2006.
Oops, I'll fix this.
And it does not list/link all the tests.
I will add them.
Mis spelling of 'hexidecimal' The external representation of a uuid is a string of hexidecimal digits
I'll fix this. (How do others spell check html?)
Note: boost::uuids::uuid::size() always returnes 16. Mispelled 'returnes'
I'll fix this too.
Is there a reason why create does not also take a std::string (with default length .size() as parameter?) // Static functions static uuid create(uuid const& namespace_uuid, char const* name, int name_length);
I assumed it would exist and was surprised when it didn't.
(Should the name_length have a default value? C-string size - 1?)
I don't have a preference. The create function was done this way so that it could take a block of memory and not just strings, but thinking about it now, void* would be better for this. It does not sound like this is useful and I should just have the function take a std::basic_string. I've also considered changing this to a function object similar to basic_uuid_generator instead of a static function. What do people want?
I also believe that a really basic example would be useful. This helps novices.
A little demo I knocked up quickly attached. (MSVC 8 Sp1)
With your permission, I'll include your example.
It reveals that the hated 4996 warnings are triggered (at default MS warning level 3). I think these need to be suppressed with push'n'popping.
Also I got a faceful of these at warning level 4. Again they should be suppressed. 1>I:\boost_1_37_0\boost/random/detail/pass_through_engine.hpp(49) : warning C4512: 'boost::random::detail::pass_through_engine<UniformRandomNumberGenerator>' : assignment operator could not be generated
I will suppress these warnings as in your example.
1>H:\uuid\boost/uuid/uuid.hpp(364) : warning C4244: '=' : conversion from 'int' to 'char', possible loss of data
Should be silenced using a static_cast?
c = static_cast<ch>(is.peek());
I will fix this.
But overall this seems 'OK to ship'.
HTH
Paul
--- Paul A. Bristow Prizet Farmhouse Kendal, UK LA8 8AB +44 1539 561830, mobile +44 7714330204 pbristow@hetp.u-net.com
Thanks, Andy Tompkins.
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Andy Tompkins Sent: 25 November 2008 01:59 To: boost_dev Subject: Re: [boost] [Review] UUID library (mini-)review starts today, November 23rd
I'll fix this. (How do others spell check html?)
Well if you write in Quickbook, you can use your regular spell checker - I use Textpad for example. (It also produces hyperlinked pdfs - and should include automatic indexing soon). But of course it produces lots of false alarms :-( (But don't redo the docs just for this).
Is there a reason why create does not also take a std::string (with default length .size() as parameter?) // Static functions static uuid create(uuid const& namespace_uuid, char const* name, int name_length);
I assumed it would exist and was surprised when it didn't.
(Should the name_length have a default value? C-string size - 1?)
I don't have a preference. The create function was done this way so that it could take a block of memory and not just strings, but thinking about it now, void* would be better for this. It does not sound like this is useful and I should just have the function take a std::basic_string. I've also considered changing this to a function object similar to basic_uuid_generator instead of a static function. What do people want?
Are these mutually exclusive? I don't have a strong view - I'm just reported what I assumed. (But then assumption is the mother of all foul-ups ;-)
I also believe that a really basic example would be useful. This helps novices.
With your permission, I'll include your example.
Of course - see licence ;-) You should (be able to) remove the #defines if you sort out the details below. And other usages would be useful - For example, creating unique filenames... Examples are often more useful than manuals.
I will suppress these warnings as in your example. Good.
Paul --- Paul A. Bristow Prizet Farmhouse Kendal, UK LA8 8AB +44 1539 561830, mobile +44 7714330204 pbristow@hetp.u-net.com
On Tue, 25 Nov 2008 09:36:44 -0000, "Paul A. Bristow" <pbristow@hetp.u- net.com> said:
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost- bounces@lists.boost.org] On Behalf Of Andy Tompkins Sent: 25 November 2008 01:59 To: boost_dev Subject: Re: [boost] [Review] UUID library (mini-)review starts today, November 23rd
I'll fix this. (How do others spell check html?)
Well if you write in Quickbook, you can use your regular spell checker - I use Textpad for example. (It also produces hyperlinked pdfs - and should include automatic indexing soon).
But of course it produces lots of false alarms :-(
(But don't redo the docs just for this).
I'm hoping to have someone else convert the docs for me if the uuid library is accepted.
Is there a reason why create does not also take a std::string (with default length .size() as parameter?) // Static functions static uuid create(uuid const& namespace_uuid, char const* name, int name_length);
I assumed it would exist and was surprised when it didn't.
(Should the name_length have a default value? C-string size - 1?)
I don't have a preference. The create function was done this way so that it could take a block of memory and not just strings, but thinking about it now, void* would be better for this. It does not sound like this is useful and I should just have the function take a std::basic_string. I've also considered changing this to a function object similar to basic_uuid_generator instead of a static function. What do people want?
Are these mutually exclusive? I don't have a strong view - I'm just reported what I assumed. (But then assumption is the mother of all foul-ups ;-)
No, they are not mutually exclusive. To me, having both functions is a stronger reason to change the interface from static functions to a function object.
I also believe that a really basic example would be useful. This helps novices.
With your permission, I'll include your example.
Of course - see licence ;-)
Thanks!
You should (be able to) remove the #defines if you sort out the details below.
And other usages would be useful - For example, creating unique filenames... Examples are often more useful than manuals.
Good idea. I will put this example in and others as they come up.
I will suppress these warnings as in your example. Good.
Paul
--- Paul A. Bristow Prizet Farmhouse Kendal, UK LA8 8AB +44 1539 561830, mobile +44 7714330204 pbristow@hetp.u-net.com
Thanks, Andy Tompkins
Hartmut Kaiser wrote:
- What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With what compiler? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain?
Regards Hartmut Review Manager
Herewith my non-exhaustive review: I've looked at the library, good work, it looks very interesting. I did a quick reading, and in particular, I paid attention to the many of its intended uses (in the Rationale), and how this would have to be done in practice (the Interface). * The design notes state that it is based on X.667-E. This document describes three construction mechanisms: time-based, rng-based, and string-based. The proposal doesn't have the time-based constructor. Is there a reason why this is missing? * Is the boost namespace that cluttered that it isn't possible to hold "boost::uuid" in it? I think boost::uuids::uuid is kind of repetitive. (Or is there a policy against putting stuff in the boost root-namespace?) * Construction is described in "Constructors" and "Construction" -- this could perhaps lead to confusion? * In Representation: Is there a reason for a .to_string() member function instead of an operator std::string()? I.e., it reduces std::string s = u.to_string() to std::string s( u ); * Considering the examples in the Rationale and the available constructors: I'm curious how to easily "tag an object" using the provided constructors. By a random number? Or could I just pass the address of the object and use a string-based method? In other words, it would be nice if such an example from the Rationale would be topic of an example. I vote for acceptance into boost, given that just a bit more attention is paid to a potential user's convenience. Kind regards, Rutger
On Tue, 25 Nov 2008 20:03:22 +0100, "Rutger ter Borg" <rutger@terborg.net> said: < snip >
Herewith my non-exhaustive review:
I've looked at the library, good work, it looks very interesting. I did a quick reading, and in particular, I paid attention to the many of its intended uses (in the Rationale), and how this would have to be done in practice (the Interface).
* The design notes state that it is based on X.667-E. This document describes three construction mechanisms: time-based, rng-based, and string-based. The proposal doesn't have the time-based constructor. Is there a reason why this is missing?
Only because I'm not sure how to implement the time-based construction mechanism a portable way.
* Is the boost namespace that cluttered that it isn't possible to hold "boost::uuid" in it? I think boost::uuids::uuid is kind of repetitive. (Or is there a policy against putting stuff in the boost root-namespace?)
I did this because of http://www.boost.org/development/requirements.html#Naming_consistency But I have no problem doing something else if this list wants me too.
* Construction is described in "Constructors" and "Construction" -- this could perhaps lead to confusion?
Do you mean "Constructors" and "Creation"? Hmm, I'll give this some thought.
* In Representation: Is there a reason for a .to_string() member function instead of an operator std::string()? I.e., it reduces std::string s = u.to_string() to std::string s( u );
I tend to avoid implicit conversions. One can also use boost::lexical_cast<std::string>(uuid). Again, if this list wants operator std::string() and the like, I will add it.
* Considering the examples in the Rationale and the available constructors: I'm curious how to easily "tag an object" using the provided constructors. By a random number? Or could I just pass the address of the object and use a string-based method? In other words, it would be nice if such an example from the Rationale would be topic of an example.
I think an example for this is a good idea. On way would be: class Foo { public: Foo() // create a temporary generator (or pass one in the constructor // or create a singleton) and initialize m_uuid with it. : m_uuid(boost::uuids::uuid_generator()()) {} private: const boost::uuids::uuid m_uuid; };
I vote for acceptance into boost, given that just a bit more attention is paid to a potential user's convenience.
Kind regards,
Rutger
Thanks, Andy.
AMDG Andy Tompkins wrote:
* In Representation: Is there a reason for a .to_string() member function instead of an operator std::string()? I.e., it reduces std::string s = u.to_string() to std::string s( u );
I tend to avoid implicit conversions. One can also use boost::lexical_cast<std::string>(uuid). Again, if this list wants operator std::string() and the like, I will add it.
I don't think an implicit conversion here would be a good idea. In Christ, Steven Watanabe
On Tue, 25 Nov 2008 20:05:35 -0800, "Steven Watanabe" <watanabesj@gmail.com> said:
AMDG
Andy Tompkins wrote:
* In Representation: Is there a reason for a .to_string() member function instead of an operator std::string()? I.e., it reduces std::string s = u.to_string() to std::string s( u );
I tend to avoid implicit conversions. One can also use boost::lexical_cast<std::string>(uuid). Again, if this list wants operator std::string() and the like, I will add it.
I don't think an implicit conversion here would be a good idea.
I agree.
In Christ, Steven Watanabe
Regards, Andy Tompkins
Andy Tompkins wrote:
Only because I'm not sure how to implement the time-based construction mechanism a portable way.
I'm not sure what is required but perhaps the newly sandboxed Chrono library can help here?
I tend to avoid implicit conversions. One can also use boost::lexical_cast<std::string>(uuid). Again, if this list wants operator std::string() and the like, I will add it.
Please no. -- Michael Marcin
On Wed, 26 Nov 2008 11:14:38 -0600, "Michael Marcin" <mike.marcin@gmail.com> said:
Andy Tompkins wrote:
Only because I'm not sure how to implement the time-based construction mechanism a portable way.
I'm not sure what is required but perhaps the newly sandboxed Chrono library can help here?
I will take a look at Chrono, it may help. The time-based construction ideally requires a MAC address, and I don't know how to get it portably.
I tend to avoid implicit conversions. One can also use boost::lexical_cast<std::string>(uuid). Again, if this list wants operator std::string() and the like, I will add it.
Please no.
This is the second no vote, plus mine makes 3. I won't add an implicit conversion to std::string.
-- Michael Marcin
Regards, Andy Tompkins
Only because I'm not sure how to implement the time-based construction mechanism a portable way.
I'm not sure what is required but perhaps the newly sandboxed Chrono library can help here?
I will take a look at Chrono, it may help. The time-based construction ideally requires a MAC address, and I don't know how to get it portably.
Why not leaving the burden of retrieving the MAC address to the user, then? Regards Hartmut
Hartmut Kaiser wrote:
Only because I'm not sure how to implement the time-based construction mechanism a portable way.
I'm not sure what is required but perhaps the newly sandboxed Chrono library can help here? I will take a look at Chrono, it may help. The time-based construction ideally requires a MAC address, and I don't know how to get it portably.
Why not leaving the burden of retrieving the MAC address to the user, then?
That was what I was going to suggest too.
Regards Hartmut
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On Thu, 27 Nov 2008 06:50:37 -0600, "Hartmut Kaiser" <hartmut.kaiser@gmail.com> said:
Only because I'm not sure how to implement the time-based construction mechanism a portable way.
I'm not sure what is required but perhaps the newly sandboxed Chrono library can help here?
I will take a look at Chrono, it may help. The time-based construction ideally requires a MAC address, and I don't know how to get it portably.
Why not leaving the burden of retrieving the MAC address to the user, then?
That is a _really_ good idea! I will spend some time and take a serious look at implementing a time-based uuid generator.
Regards Hartmut
Thanks, Andy Tompkins
Hartmut Kaiser wrote:
Only because I'm not sure how to implement the time-based construction mechanism a portable way.
I'm not sure what is required but perhaps the newly sandboxed Chrono library can help here? I will take a look at Chrono, it may help. The time-based construction ideally requires a MAC address, and I don't know how to get it portably.
Why not leaving the burden of retrieving the MAC address to the user, then?
Because portably retrieve a good MAC address is hard and I don't want to have to implement that logic. My laziness aside it probably is reasonable to say this is outside of the scope this library. Perhaps another utility library to obtain the mac address can be authored by one of the people requesting this feature? They are probably going to have to implement that code to use it anyways. -- Michael Marcin
On Thu, 27 Nov 2008 18:03:45 +0100, "Rutger ter Borg" <rutger@terborg.net> said:
Andy Tompkins wrote:
I will take a look at Chrono, it may help.
The ptime class in the Boost.Date_Time library also provides different portable constructors based on the system's time.
It looks like Boost.DateTime may be a better fit and it is already in Boost. I need to get the current time in UTC (hopefully quite accurately).
Kind regards,
Rutger
Thanks, Andy Tompkins
Why not leaving the burden of retrieving the MAC address to the user, then?
In my opinion one can provide a "generic" generator, which requires a MAC address; but there should be a set of platform specific generators, which use system calls to get the hardware address. I am not a network expert, but it seems that on Linux you can obtain the MAC address through IOCTL http://www.geekpage.jp/en/programming/linux-network/get-macaddr.php even if this approach couldn't be very reliable http://unix.derkeiler.com/Newsgroups/comp.unix.solaris/2007-06/msg00854.html In any case one could look at the ifconfig source http://www.koders.com/c/fidF4E339CD5847BBECCA5405DD6592D79452A56D3E.aspx?s=l... (I am not sure it is the original code :-) For Windows user I found the article http://www.codeguru.com/cpp/i-n/network/networkinformation/article.php/c5451 with a warning http://www.codeguru.com/cpp/i-n/network/networkinformation/comments.php/c545... In any cases, one should add the error checking :-) Manuel Fiorelli
Manuel Fiorelli wrote:
Why not leaving the burden of retrieving the MAC address to the user, then?
For Windows user I found the article
http://www.codeguru.com/cpp/i-n/network/networkinformation/article.php/c5451
with a warning
http://www.codeguru.com/cpp/i-n/network/networkinformation/comments.php/c545...
I can add another one (although I'm not sure how the OS behaves in that case): if you actually want something unique, you cannot blindly take the first MAC you get, you need to be smarter (and unfortunately I don't know how to do that). I recently saw the case where a developer thought he was getting something unique by retrieving the first MAC address of the machine... only to discover that on machines with VMWare installed, the first two returned MAC addresses are actually the MAC addresses of VMWare's virtual network adapters, which are (by default) the same everywhere. IOW, his code was returning the same MAC address on every PC with VMWare installed. François
Francois Barel wrote:
I recently saw the case where a developer thought he was getting something unique by retrieving the first MAC address of the machine... only to discover that on machines with VMWare installed, the first two returned MAC addresses are actually the MAC addresses of VMWare's virtual network adapters, which are (by default) the same everywhere. IOW, his code was returning the same MAC address on every PC with VMWare installed.
François
I wasn't aware of that issue. Things are yet complicated, but your point complicates them much more. Manuel Fiorelli
On Fri, 28 Nov 2008 17:40:44 +0100, "Manuel Fiorelli" <manuel.fiorelli@gmail.com> said:
Francois Barel wrote:
I recently saw the case where a developer thought he was getting something unique by retrieving the first MAC address of the machine... only to discover that on machines with VMWare installed, the first two returned MAC addresses are actually the MAC addresses of VMWare's virtual network adapters, which are (by default) the same everywhere. IOW, his code was returning the same MAC address on every PC with VMWare installed.
François
I wasn't aware of that issue. Things are yet complicated, but your point complicates them much more.
Wow complicated indeed. I agree that one could make a generic uuid generator that is given that MAC address. And then create some platform specific generators that work on top of the generic one. But the generic uuid generator will still have to be in the public interface for those complicated cases.
Manuel Fiorelli
Thanks, Andy Tompkins
Having seen all these issues, I consider that the platform specific generator could be deferred until one discovers how to construct them correctly. Isn't there some one, who knows how to get the MAC address? Manuel Fiorelli
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of Manuel Fiorelli Sent: 29 November 2008 20:38 To: boost@lists.boost.org Subject: Re: [boost] [Review] UUID library (mini-)review starts today, November 23rd
Having seen all these issues, I consider that the platform specific generator could be deferred until one discovers how to construct them correctly.
Isn't there some one, who knows how to get the MAC address?
There is plenty of info, it sounds as though getting a MAC address may be iffy, and not portable. So would it not still be useful to provide an implementation that only goes as far as expecting a MAC address as a parameter? Leaving the user to provide the MAC address. That doesn't preclude later adding an overloaded function which does it 'automatically' - if that is ever possible/desirable. Paul --- Paul A. Bristow Prizet Farmhouse Kendal, UK LA8 8AB +44 1539 561830, mobile +44 7714330204 pbristow@hetp.u-net.com
Here goes my little review: - What is your evaluation of the design? I like the overall design of the library. However, I think that the library should be better templatized by the string class and document the string concept it uses. This would allow users of the library to switch to another string class implementation compatible with the used string concept (std::string). For convenience it could provide a typedef for the std::string class. IMHO, the library would be more useful if it provides an interface for generating uuids with different technologies: - random number generator - time based, MAC - OS Different use cases may have different constraints a generator must fullfill. To be useful the library should document the differences between the generators. It should especially doucument the guarantees that the different generators provide (security, speed, memory consumption, ...). That said, I do not think that the library should be rejected for missing of actual generators. It should solely provide an interface which allows later contributers to add new uuid generators. I would however, appreciate at least the OS generator case. - What is your evaluation of the implementation? Clean and readable. I'm fine with it. - What is your evaluation of the documentation? I think that it should give a little more background about the generator algorithm it uses. Additionally, given that there are different generators provided, I would like to see a comparsion in the performance and security domain. - What is your evaluation of the potential usefulness of the library? This is (at least for me) a must have library. Boost should have such a component. It is an usefull library in the form it exists today. - Did you try to use the library? With what compiler? Did you have any problems? I'm using the library already in our CAD application. It works fine for us. We use the MS vc9 sp1 compiler. Currently, we do not have any problems with the library. - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? Some time. - Are you knowledgeable about the problem domain? Hmm, no not really. I'm a user and I do need such a component with a simple, stable interface. I vote Yes, to include the UUID library in Boost. Regards, Johannes
On Tue, 2 Dec 2008 11:12:42 +0100, "Johannes Brunen" <JBrunen@DataSolid.de> said:
Here goes my little review:
- What is your evaluation of the design?
I like the overall design of the library. However, I think that the library should be better templatized by the string class and document the string concept it uses. This would allow users of the library to switch to another string class implementation compatible with the used string concept (std::string). For convenience it could provide a typedef for the std::string class.
I would like to remove the constructors that take a string to generators. I would also like to remove the to_..._string member functions and make some free extraction/conversion functions.
IMHO, the library would be more useful if it provides an interface for generating uuids with different technologies: - random number generator - time based, MAC - OS Different use cases may have different constraints a generator must fullfill. To be useful the library should document the differences between the generators. It should especially doucument the guarantees that the different generators provide (security, speed, memory consumption, ...).
Agreed.
That said, I do not think that the library should be rejected for missing of actual generators. It should solely provide an interface which allows later contributers to add new uuid generators. I would however, appreciate at least the OS generator case.
- What is your evaluation of the implementation?
Clean and readable. I'm fine with it.
- What is your evaluation of the documentation?
I think that it should give a little more background about the generator algorithm it uses. Additionally, given that there are different generators provided, I would like to see a comparsion in the performance and security domain.
I will do this.
- What is your evaluation of the potential usefulness of the library?
This is (at least for me) a must have library. Boost should have such a component. It is an usefull library in the form it exists today.
- Did you try to use the library? With what compiler? Did you have any problems?
I'm using the library already in our CAD application. It works fine for us. We use the MS vc9 sp1 compiler. Currently, we do not have any problems with the library.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Some time.
- Are you knowledgeable about the problem domain?
Hmm, no not really. I'm a user and I do need such a component with a simple, stable interface.
I vote Yes, to include the UUID library in Boost.
Regards, Johannes
Thanks, Andy Tompkins
[Sorry for the lateness. Haven't looked at any other reviews yet.] On Nov 23, 2008, at 7:13 PM, Hartmut Kaiser wrote:
The mini-review of Andy Tompkins UUID library starts today, November 23rd 2008, and will end on November 30th. I really hope to see your vote and your participation in the discussions on the Boost mailing lists!
The library can be downloaded from the vault here (it's the file uuid_v13.zip in the topmost directory): http://tinyurl.com/6xx28b
The initial review of the UUID library ended with a provisional acceptance (read here: http://article.gmane.org/gmane.comp.lib.boost.user/ 27774/).
This mini review is meant to allow for a final decision after looking at the changed parts of the library. Here is a list of things fixed and changed:
- fixed the licensing issues revealed by the initial review - fixed the security vulnerability revealed by the initial review - renamed to uuid, moved all classes, functions, etc. to namespace boost::uuids - implemented sha1 hash function (thus no license problem)
Others, including myself, are working on encoding libraries. If this library, and an encoding library are added, we could change the implementation to use the new encoding library.
- new class basic_uuid_generator to create random-based uuids. The random number generator is no longer hard coded. It can use any random number generator, default is boost::mt19937 - implemented a good seed function for random number generators - all functions are now reentrant, all classes are as thread safe as an int and the library is no longer dependent on Boost.Thread
---------------------------------------------------
Please always state in your review, whether you think the library should be accepted as a Boost library!
Yes, please accept it.
Additionally please consider giving feedback on the following general topics:
- What is your evaluation of the design?
Why is there a container interface for inspecting the value's octets? For a similar problem in my code (the MD5 stuff in the Sandbox SVN), I defined a copying-out member function template that takes an output iterator and returns the updated version of same. You could do that, making sure to define some (compile-time) item indicating the length (16), to facilitate easier changes to internal storage. Don't some operating systems provide UUID generation? Could there be a way to add a creation function that uses the OS's code? Is default construction useful besides when initialization can't be done in one step? Instead of an "is_null" member function, maybe (pseudo-)Boolean conversion (and "operator not") can be used. I don't think the multitude of string conversion techniques are needed. Keep the constructor with "char const *" for pseudo- literals. Everything else, in either direction, could use "lexical_cast".
- What is your evaluation of the implementation?
The "generator_iterator" substitute in "seed_rng.hpp" doesn't completely help. It is supposed to improve on the version in "boost/ generator_iterator.hpp" in the area of end-iterator support. I originally said more here, but I erased it because when I filed a ticket on this issue, I found one already (#2428) and put my notes there. The implementation of the constructor that takes a byte-returning input-iterator shows why we need a double-bounded copy algorithm. (Now in new ticket #2578.) The copy constructor, destructor, and copy-assignment operator don't do anything different than the automatically-defined versions would. So they could be omitted. Maybe the "create_name_based" and "get_showbraces_index" member functions need to be in a new mandatory source file. If so, you could move some common error strings there too. BTW, why do "create" (and "create_name_based") need the length based in as a separate argument? Just use "std::strlen". The I/O function templates could be tightened up some. (I noticed it since I work on I/O quite a bit. It could be done later.) The optimization would be more important if my dump-most-of-the-string- conversions-for-lexical_cast idea is used. Should the serialization be done in a separate header? BTW, how is a custom primitive type handled? Is it just a byte-level save/load of memory? (What if the type isn't POD, like this one?)
- What is your evaluation of the documentation?
It's adequate, but maybe detailed Doxygen information on each item should be added.
- What is your evaluation of the potential usefulness of the library?
It can be useful for (temporary) IDs. We have to make sure that the type, especially its object representation, doesn't get heavy (i.e. stay quasi-POD).
- Did you try to use the library? With what compiler? Did you have any problems?
I used it on Mac OS X 10.4 (PowerPC) with XCode. All the tests, and Paul Bristow's example, seemed to work.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Two days of reading and trying out the code. Not too much of the theory, though.
- Are you knowledgeable about the problem domain?
Not really. -- Daryle Walker Mac, Internet, and Video Game Junkie darylew AT hotmail DOT com
participants (11)
-
Andy Tompkins -
Daryle Walker -
Francois Barel -
Hartmut Kaiser -
Johannes Brunen -
Manuel Fiorelli -
Michael Marcin -
Paul A. Bristow -
Rutger ter Borg -
Stefano Delli Ponti -
Steven Watanabe