qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Stitcher <astitc...@redhat.com>
Subject Re: [c++]: messaging api, uuids and handles
Date Fri, 15 Jan 2010 17:07:07 GMT
On Fri, 2010-01-15 at 08:53 -0500, Alan Conway wrote:
> On 01/15/2010 07:15 AM, Gordon Sim wrote:
> > Any comments on the attached (re)implementation?
> >
> >
> 
> === .cpp:
> 
> Assignment needs a self-assignment test:
> Uuid& Uuid::operator=(const Uuid& other)
> {
>      if (this == &other) return;
>      ::memcpy(data, other.data, UUID_SIZE);
>      return *this;
> }

Why not use ::uuid_copy() instead of ::memmove()

> 
> Needs operator <  (and for completeness >, <=,  >=)
> 
> Operator ==, < etc. can use uuid_compare()	

Agree with use of ::uuid_compare() - it's pretty simple to implement the
comparisons using it.

> 
> === .h
> 
> Replace #include istream/ostream in .h with
> 
> #include <iosfwd>
> 
> It forward declares istream & ostream without pulling in the full declarations, 
> so it is less compileoverhead for compilation units that use UUIDs but don't 
> actually use istream/ostream.
> You'll need to #include istream/ostream in the .cpp
> 
> .h add doc comments to ctor explain bool unique.
> 
> Not sure about leaving data public. If you do make it private you need to add 
> operations to copy UUIDs to/from char arrays since the user may want to 
> store/retrive UUIDs in binary form. You could argue it's OK to leave the data in 
> the public API since its part of the ABI anyway, but it looks a bit odd since we 
> generally don't have any public data on our classes, even the value classes. I'd 
> be inclined to stick with convention at the expense of adding:
> 
> class Uuid {
> public:
>    static const size_t SIZE=16;

There's already:

static const size_t UUID_SIZE=16;

probably want to move this into the header file to use it in the header
file.

so this isn't necessary.

>    /** Copy the UUID from data16, which must point to a 16-byte UUID */
>    Uuid(unsigned char* data16);
>    size_t size() const { return SIZE; } // For compatibility with std sequences.
>    const unsigned char* data() const { return array; }
> 
> private
>    const char* array[SIZE];

This is wrong BTW should be "char array[SIZE]"

> }

Not sure about using a char* to pass in raw data. This is C++ not C so
you can tell it the interface is a char[16] directly (unless I've got
seriously confused) how about:


class Uuid {
...
public:
	Uuid(const char data[16]); // or [UUID_SIZE]
...
	size_t size() const; // or inline { return UUID_SIZE; }
	const char& data()[16] const;

{that syntax looks wrong but I'm trying to return a reference to a
constant char[16]}

Andrew



---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project:      http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org


Mime
View raw message