Re: [boost] [review] Boost.Guid

From: Andy (atompkins_at_[hidden])
Date: 2007-05-07 11:19:06

"Peter Dimov" <pdimov_at_[hidden]> wrote in
news:00dd01c78f17$ba7e5610$6407a80a_at_pdimov2: 

> I think that the library is useful, needed and should be accepted
> subject to the comments below.
> 
> * The proper (standard) name for the component is UUID. GUID is a 
> Microsoft-specific term for the same thing, which has later been
> adopted to mean any kind of globally unique identifier. We should
> stick to the standard name unless there are clear reasons to avoid it.

This has been suggested before and I will gladly change it to uuid.

> 
> * I agree that the time(0) seed is unacceptable. A good source of
> entropy probably deserves its own library. It's also not easy to make
> it header-only. One compromise could be for the create function to
> take an Engine argument, but this takes away the simplicity.

See another post.
http://article.gmane.org/gmane.comp.lib.boost.devel/158740
I hope that the defaults will keep the simplicity. 

> 
> * The dependency on Boost.Threads in its present form can be dropped
> since it doesn't make the library thread safe. There are a number of
> function static variables, many of them unnecessary.

Agreed.

> 
> * The string constructors should probably be optimized to not use
> iostream extraction, but this is an implementation detail.

I can look into this.

> 
> * The guid class is almost a container, but not quite. It is not clear
> why output_bytes has been preferred over begin/end returning const
> iterators. These could come for free from the underlying array<>.

Great idea!  I didn't want to make it easy to directly change values.  I
didn't think of providing just const iterators. 

> 
> * The dependency on uint8_t is unnecessary; the underlying type should
> be unsigned char. There is no need to insist on exactly 8 bits.

Ok.  If unsigned char is more than 8 bits then sizeof(guid) would be
larger.  Maybe this is not an issue. 

> 
> * The c == '{' comparison in operator>> seems to be missing a 'widen'
> call. There is no guarantee that '{' and L'{' compare equal.

Thanks.  I get a little lost with this sort of stuff sometimes.  Just to
help me, who provides the 'widen' function?  The stream? 

> 
> * It isn't clear why a guid is marked as a primitive type for
> serialization purposes. Not doing so would allow the macro
> BOOST_GUID_ENABLE_SERIALIZATION to be dropped since
> 
>     friend class boost::serialization::access;
> 
>     template<class Archive>
>     void serialize(Archive &ar, const unsigned int /* file_version
>     */){ 
>         ar & data_;
>     }
> 
> does not seem to need any serialization headers and is therefore
> zero-cost. 

I want to read the other post about this before I reply.

Andy.