I reviewed the new C++ API. Overall looks very clean, I have a few suggestions. My comments are in the attached diff - apply & grep for "FIXME aconway".
commit 29b27d442687b43341b73779adf25c5be846da06 Author: Alan Conway <[email protected]> Date: Tue Mar 16 12:58:25 2010 -0400 Comments on the new C++ API. diff --git a/qpid/cpp/include/qpid/messaging/Address.h b/qpid/cpp/include/qpid/messaging/Address.h index f3ca30b..e469d15 100644 --- a/qpid/cpp/include/qpid/messaging/Address.h +++ b/qpid/cpp/include/qpid/messaging/Address.h @@ -30,6 +30,8 @@ namespace qpid { namespace messaging { +// FIXME aconway 2010-03-16: doc comment on functions that can throw these. +// What's the difference between invalid and malformed? struct InvalidAddress : public qpid::Exception { InvalidAddress(const std::string& msg); @@ -42,6 +44,9 @@ struct MalformedAddress : public qpid::Exception class AddressImpl; +// FIXME aconway 2010-03-16: doc comment should state that Address has +// value semantics. + /** * Represents an address to which messages can be sent and from which * messages can be received. Often a simple name is sufficient for @@ -157,20 +162,29 @@ class Address QPID_CLIENT_EXTERN void setName(const std::string&); QPID_CLIENT_EXTERN const std::string& getSubject() const; QPID_CLIENT_EXTERN void setSubject(const std::string&); + // FIXME aconway 2010-03-16: how is hasSubject() different from + // !getSubject().empty()? If it's the same, then remove hasSubject. + // If its different then we need additional ctors & setters to allow + // creating an Address without a subject and unsetting the subject. QPID_CLIENT_EXTERN bool hasSubject() const; QPID_CLIENT_EXTERN const Variant::Map& getOptions() const; QPID_CLIENT_EXTERN Variant::Map& getOptions(); QPID_CLIENT_EXTERN void setOptions(const Variant::Map&); + // FIXME aconway 2010-03-16: what is type for? QPID_CLIENT_EXTERN std::string getType() const; QPID_CLIENT_EXTERN void setType(const std::string&); + // FIXME aconway 2010-03-16: superfluous, equivalent to getOptions()[key] QPID_CLIENT_EXTERN const Variant& getOption(const std::string& key) const; + // FIXME aconway 2010-03-16: suggest str() like + // std::ostringstream, or asString() like Variant. QPID_CLIENT_EXTERN std::string toStr() const; QPID_CLIENT_EXTERN operator bool() const; QPID_CLIENT_EXTERN bool operator !() const; private: + // FIXME aconway 2010-03-16: why not using AddressImpl* impl; }; diff --git a/qpid/cpp/include/qpid/messaging/Codec.h b/qpid/cpp/include/qpid/messaging/Codec.h index bacec5c..66272a0 100644 --- a/qpid/cpp/include/qpid/messaging/Codec.h +++ b/qpid/cpp/include/qpid/messaging/Codec.h @@ -28,6 +28,14 @@ namespace qpid { namespace messaging { class Variant; +// FIXME aconway 2010-03-16: doc commnt describe role of this class, +// is it @internal or might users derive from it? + +// FIXME aconway 2010-03-16: strong reservations about pinning +// ourselves to std::string for encoding/decoding - it robs us of all +// control over memory allocation which is likely to be a critical +// performance factor. + /** * */ diff --git a/qpid/cpp/include/qpid/messaging/Connection.h b/qpid/cpp/include/qpid/messaging/Connection.h index e2d1cc2..cedf621 100644 --- a/qpid/cpp/include/qpid/messaging/Connection.h +++ b/qpid/cpp/include/qpid/messaging/Connection.h @@ -38,11 +38,13 @@ namespace messaging { class ConnectionImpl; class Session; +// FIXME aconway 2010-03-16: doc comment functions that can throw this. struct InvalidOptionString : public qpid::Exception { InvalidOptionString(const std::string& msg); }; +// FIXME aconway 2010-03-16: doc comment: has reference semantics class Connection : public qpid::client::Handle<ConnectionImpl> { public: @@ -84,8 +86,11 @@ class Connection : public qpid::client::Handle<ConnectionImpl> QPID_CLIENT_EXTERN ~Connection(); QPID_CLIENT_EXTERN Connection& operator=(const Connection&); QPID_CLIENT_EXTERN void setOption(const std::string& name, const Variant& value); + // FIXME aconway 2010-03-16: why no getOption? QPID_CLIENT_EXTERN void open(const std::string& url); QPID_CLIENT_EXTERN void close(); + // FIXME aconway 2010-03-16: suggest newTransactionalSession(name) + // It's more descriptive than newSession(true, name) QPID_CLIENT_EXTERN Session newSession(bool transactional, const std::string& name = std::string()); QPID_CLIENT_EXTERN Session newSession(const std::string& name = std::string()); QPID_CLIENT_EXTERN Session newSession(const char* name); diff --git a/qpid/cpp/include/qpid/messaging/ListContent.h b/qpid/cpp/include/qpid/messaging/ListContent.h index f57a920..8c27977 100644 --- a/qpid/cpp/include/qpid/messaging/ListContent.h +++ b/qpid/cpp/include/qpid/messaging/ListContent.h @@ -24,6 +24,30 @@ #include "qpid/client/ClientImportExport.h" #include "Variant.h" +// FIXME aconway 2010-03-16: +/* + IMO the content and view classes contribute nothing. They don't + provide usable abstraction over Variant::Map and Variant::List + because they publicly expose the iterators of those classes. + + Suggest replacing them with free convenience functions as follows: + + void decode(const Message&, Variant::Map&); + void decode(const Message&, Variant::List&); + void encode(const Variant::Map&, Message&); + void encode(const Variant::List&, Message&); + + So instead of: + MapContent m(msg); do stuff with m; m.encode() + user does + Map m; + decode(msg, m); do stuff with m; encode(m, msg) + + IMO the latter is clearer because its obvious you have to encode m + if you want changes to get back into the message, whereas the former + may give the impression of being a proxy for the message content. +*/ + namespace qpid { namespace messaging { diff --git a/qpid/cpp/include/qpid/messaging/ListView.h b/qpid/cpp/include/qpid/messaging/ListView.h index 4970a20..dc71ac6 100644 --- a/qpid/cpp/include/qpid/messaging/ListView.h +++ b/qpid/cpp/include/qpid/messaging/ListView.h @@ -31,6 +31,7 @@ namespace messaging { class ListViewImpl; class Message; +// FIXME aconway 2010-03-16: see comment in ListContent.h /** * Provides a view of message content as a list */ diff --git a/qpid/cpp/include/qpid/messaging/MapContent.h b/qpid/cpp/include/qpid/messaging/MapContent.h index 3a80a38..9296698 100644 --- a/qpid/cpp/include/qpid/messaging/MapContent.h +++ b/qpid/cpp/include/qpid/messaging/MapContent.h @@ -33,6 +33,7 @@ namespace messaging { class MapContentImpl; class Message; +// FIXME aconway 2010-03-16: see comment in ListContent.h /** * Allows message content to be manipulated as a map */ diff --git a/qpid/cpp/include/qpid/messaging/MapView.h b/qpid/cpp/include/qpid/messaging/MapView.h index 910dfca..9ee2cce 100644 --- a/qpid/cpp/include/qpid/messaging/MapView.h +++ b/qpid/cpp/include/qpid/messaging/MapView.h @@ -32,6 +32,7 @@ namespace messaging { class MapViewImpl; class Message; +// FIXME aconway 2010-03-16: see comment in ListContent.h /** * Provides a view of message content as a list */ diff --git a/qpid/cpp/include/qpid/messaging/Message.h b/qpid/cpp/include/qpid/messaging/Message.h index 30e15d7..ea7db1b 100644 --- a/qpid/cpp/include/qpid/messaging/Message.h +++ b/qpid/cpp/include/qpid/messaging/Message.h @@ -37,6 +37,7 @@ class Address; class Codec; struct MessageImpl; +// FIXME aconway 2010-03-16: doc comment: note that messages have value semantics. /** * Representation of a message. */ @@ -85,12 +86,45 @@ class Message QPID_CLIENT_EXTERN const Variant::Map& getHeaders() const; QPID_CLIENT_EXTERN Variant::Map& getHeaders(); + // FIXME aconway 2010-03-16: see below for suggested replacement of getContent QPID_CLIENT_EXTERN const std::string& getContent() const; QPID_CLIENT_EXTERN std::string& getContent(); + QPID_CLIENT_EXTERN void setContent(const std::string&); + // FIXME aconway 2010-03-16: doc comment to specify that we copy chars. QPID_CLIENT_EXTERN void setContent(const char* chars, size_t count); + + // FIXME aconway 2010-03-16: see below for suggested replacement of getContent QPID_CLIENT_EXTERN void getContent(std::pair<const char*, size_t>& content) const; + +#if 0 // FIXME aconway 2010-03-16: Propsoed replacement for the getContent functions. + + // Rationale: We don't want to return a std::string&, that forces + // us to store the content in a std string internally. Return + // string by value instead (which is almost as efficient if we + // do happen to use std::string internally.) + + /** Get the content of the message as a std::string. */ + QPID_CLIENT_EXTERN std::string getContent() const; + + // Rationale: convenience, don't require user to declare a + // pair. The user can always do pair<> p(m.getContentPtr(), + // m.getContentSize()) if desired. + // + /** Get a const pointer to the start of the content data. */ + QPID_CLIENT_EXTERN const char* getContentPtr() const; + /** Get a mutable pointer to the start of the content data. */ + QPID_CLIENT_EXTERN char* getContentPtr(); + /** Get the size of content in bytes. */ + QPID_CLIENT_EXTERN size_t getContentSize(); + + // NOTE: I don't think now is the time to introduce support for + // fragmented content storage. If/when we do, we can honour the + // above APIs by consolidating to a single buffer on demand and + // add new methods for efficient access to a framgmented message. +#endif + private: MessageImpl* impl; friend struct MessageImplAccess; diff --git a/qpid/cpp/include/qpid/messaging/Receiver.h b/qpid/cpp/include/qpid/messaging/Receiver.h index bc1f39b..4f44d3d 100644 --- a/qpid/cpp/include/qpid/messaging/Receiver.h +++ b/qpid/cpp/include/qpid/messaging/Receiver.h @@ -39,6 +39,7 @@ class Message; class ReceiverImpl; class Session; +// FIXME aconway 2010-03-16: doc reference semantics. /** * Interface through which messages are received. */ @@ -60,14 +61,16 @@ class Receiver : public qpid::client::Handle<ReceiverImpl> QPID_CLIENT_EXTERN bool get(Message& message, Duration timeout=INFINITE_DURATION); /** * Retrieves a message from this receivers local queue, or waits - * for upto the specified timeout for a message to become - * available. Throws NoMessageAvailable if there is no - * message to give after waiting for the specified timeout. + * for up to the specified timeout for a message to become + * available. + * + *...@exception NoMessageAvailable if there is no message to give + * after waiting for the specified timeout. */ QPID_CLIENT_EXTERN Message get(Duration timeout=INFINITE_DURATION); /** * Retrieves a message for this receivers subscription or waits - * for upto the specified timeout for one to become + * for up to the specified timeout for one to become * available. Unlike get() this method will check with the server * that there is no message for the subscription this receiver is * serving before returning false. @@ -79,6 +82,9 @@ class Receiver : public qpid::client::Handle<ReceiverImpl> * available. Unlike get() this method will check with the server * that there is no message for the subscription this receiver is * serving before throwing an exception. + * + *...@exception NoMessageAvailable if there is no message to give + * after waiting for the specified timeout. */ QPID_CLIENT_EXTERN Message fetch(Duration timeout=INFINITE_DURATION); /** @@ -94,11 +100,13 @@ class Receiver : public qpid::client::Handle<ReceiverImpl> * listener). */ QPID_CLIENT_EXTERN uint32_t getCapacity(); + // FIXME aconway 2010-03-16: rename getAvailable() for consistency /** * Returns the number of messages received and waiting to be * fetched. */ QPID_CLIENT_EXTERN uint32_t available(); + // FIXME aconway 2010-03-16: rename getPendingAck() for consistency /** * Returns a count of the number of messages received on this * receiver that have been acknowledged, but for which that diff --git a/qpid/cpp/include/qpid/messaging/Sender.h b/qpid/cpp/include/qpid/messaging/Sender.h index eb8ccc2..28381a8 100644 --- a/qpid/cpp/include/qpid/messaging/Sender.h +++ b/qpid/cpp/include/qpid/messaging/Sender.h @@ -49,7 +49,14 @@ class Sender : public qpid::client::Handle<SenderImpl> QPID_CLIENT_EXTERN ~Sender(); QPID_CLIENT_EXTERN Sender& operator=(const Sender&); + // FIXME aconway 2010-03-16: doc comment on exceptions. + // What's the behavior if getPending() == getCapacity()? Block or throw? + // If throw we need an exception type defined. QPID_CLIENT_EXTERN void send(const Message& message); + + /** What happens to pending messages? Discarded or sent? If sent + * does close block till they're sent? + */ QPID_CLIENT_EXTERN void close(); /** @@ -63,6 +70,7 @@ class Sender : public qpid::client::Handle<SenderImpl> * @see setCapacity */ QPID_CLIENT_EXTERN uint32_t getCapacity(); + // FIXME aconway 2010-03-16: rename getPending() for consistency. /** * Returns the number of sent messages pending confirmation of * receipt by the broker. (These are the 'in-doubt' messages). diff --git a/qpid/cpp/include/qpid/messaging/Session.h b/qpid/cpp/include/qpid/messaging/Session.h index 87f69f2..0a212db 100644 --- a/qpid/cpp/include/qpid/messaging/Session.h +++ b/qpid/cpp/include/qpid/messaging/Session.h @@ -79,19 +79,23 @@ class Session : public qpid::client::Handle<SessionImpl> */ QPID_CLIENT_EXTERN void reject(Message&); + // FIXME aconway 2010-03-16: doc comments QPID_CLIENT_EXTERN void sync(); QPID_CLIENT_EXTERN void flush(); + // FIXME aconway 2010-03-16: modified this comment, is it correct? /** - * Returns the number of messages received and waiting to be - * fetched. + * Returns the total number of messages received and waiting to be + * fetched by all Receivers belonging to this session. */ + // FIXME aconway 2010-03-16: rename getAvailable() for consistency QPID_CLIENT_EXTERN uint32_t available(); /** * Returns a count of the number of messages received this session * that have been acknowledged, but for which that acknowledgement * has not yet been confirmed as processed by the server. */ + // FIXME aconway 2010-03-16: rename getPendingAck() for consistency QPID_CLIENT_EXTERN uint32_t pendingAck(); /** * Retrieves the receiver for the next available message. If there @@ -99,14 +103,18 @@ class Session : public qpid::client::Handle<SessionImpl> * to the specified timeout waiting for one to arrive. Returns * true if a message was available at the point of return, in * which case the passed in receiver reference will be set to the - * receiver for that message or fals if no message was available. + * receiver for that message or false if no message was available. */ + // FIXME aconway 2010-03-16: Return Receiver rather than pass by + // ref? user can call isNull() on returned receiver to check for + // the no-message case. QPID_CLIENT_EXTERN bool nextReceiver(Receiver&, Duration timeout=INFINITE_DURATION); /** * Returns the receiver for the next available message. If there * are no available messages at present the call will block for up - * to the specified timeout waiting for one to arrive. Will throw - * Receiver::NoMessageAvailable if no message became available in + * to the specified timeout waiting for one to arrive. + * + *...@exception Receiver::NoMessageAvailable if no message became available in * time. */ QPID_CLIENT_EXTERN Receiver nextReceiver(Duration timeout=INFINITE_DURATION); @@ -126,13 +134,13 @@ class Session : public qpid::client::Handle<SessionImpl> QPID_CLIENT_EXTERN Receiver createReceiver(const std::string& address); /** - * Returns the sender with the specified name or throws KeyError - * if there is none for that name. + * Returns the sender with the specified name. + *...@exception KeyError if there is none for that name. */ QPID_CLIENT_EXTERN Sender getSender(const std::string& name) const; /** - * Returns the receiver with the specified name or throws KeyError - * if there is none for that name. + * Returns the receiver with the specified name. + *...@exception KeyError if there is none for that name. */ QPID_CLIENT_EXTERN Receiver getReceiver(const std::string& name) const; /** diff --git a/qpid/cpp/include/qpid/messaging/Variant.h b/qpid/cpp/include/qpid/messaging/Variant.h index 0bf62a9..e363809 100644 --- a/qpid/cpp/include/qpid/messaging/Variant.h +++ b/qpid/cpp/include/qpid/messaging/Variant.h @@ -36,6 +36,7 @@ namespace messaging { /** * Thrown when an illegal conversion of a variant is attempted. */ +// FIXME aconway 2010-03-16: doc comments on methods that throw this. struct InvalidConversion : public qpid::Exception { InvalidConversion(const std::string& msg); @@ -138,6 +139,8 @@ class Variant QPID_CLIENT_EXTERN operator int64_t() const; QPID_CLIENT_EXTERN operator float() const; QPID_CLIENT_EXTERN operator double() const; + // FIXME aconway 2010-03-16: why const char* rather than std::string? + // It's inconsistent with asString and raises memory management questions. QPID_CLIENT_EXTERN operator const char*() const; QPID_CLIENT_EXTERN operator Uuid() const; @@ -149,6 +152,8 @@ class Variant * Unlike asString(), getString() will not do any conversions and * will throw InvalidConversion if the type is not STRING. */ + // FIXME aconway 2010-03-16: implies the as*() functions do conversions? + // Need doc comments for the conversions that are done. QPID_CLIENT_EXTERN const std::string& getString() const; QPID_CLIENT_EXTERN std::string& getString();
--------------------------------------------------------------------- Apache Qpid - AMQP Messaging Implementation Project: http://qpid.apache.org Use/Interact: mailto:[email protected]
