> On Aug. 7, 2013, 7:22 p.m., Andrew Stitcher wrote: > > /trunk/qpid/cpp/include/qpid/messaging/Message.h, line 167 > > <https://reviews.apache.org/r/13328/diff/2/?file=338325#file338325line167> > > > > I wonder if the API convention here is misused. The API is pretty > > consistent in using get...() for accessors and set...() for setters. > > > > plain content() is an accessor but doesn't match the convention. > > Perhaps to it should be getVariant() or maybe asVariant(). > > Gordon Sim wrote: > Yes, I only reluctantly went for content() because I couldn't think of > anything better (and getContent() is already used). It would need to to be > getContentAsVariant() to be meaningful and that seems a bit longwinded... or > maybe getContentObject()?
I quite like getContentObject() - it indicates getContentString() which we could add to disambiguate getContent() (it would just be another name for it) > On Aug. 7, 2013, 7:22 p.m., Andrew Stitcher wrote: > > /trunk/qpid/cpp/src/qpid/types/encodings.h, line 26 > > <https://reviews.apache.org/r/13328/diff/2/?file=338357#file338357line26> > > > > I'm not sure a namespace is best here, maybe a struct with statics > > would be better here - in C++11 you can initialise static strings in class > > definition scopes, however here you will need a .cpp file. > > Gordon Sim wrote: > What's the issue with a namespace? no issue specifically, but I don't think we've introduced a namespace before simply to contain some constants. I'd usually expect constants to be in the namespace/class that uses them. In this case just qpid::types::BINARY etc. However I can see that you want to indicate that they are encodings and I guess you don't like the wordiness of qpid::types::BINARY_ENCODING preferring qpid::types::encodings::BINARY. My actual preference would be simple qpid::types::BINARY, but it is largely an aesthetic judgement. given that we have no established convention in this API. - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13328/#review24805 ----------------------------------------------------------- On Aug. 7, 2013, 7:17 p.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13328/ > ----------------------------------------------------------- > > (Updated Aug. 7, 2013, 7:17 p.m.) > > > Review request for qpid. > > > Bugs: QPID-5040 > https://issues.apache.org/jira/browse/QPID-5040 > > > Repository: qpid > > > Description > ------- > > In AMQP 1.0, though you can of course still send raw data (as a Data section) > with a content-type specified in the properties, the preferred method of > sending maps and lists is through an AmqpValue section with the type encoded > in the bytes making up that section. > > Over 0-10, the approach used was to have encode/decode functions that work > with a Variant::Map or Variant::List. The application was responsible for > encoding outgoing messages before sending them and checking the content-type > on received messages and decoding them if appropriate. > > Now that the content-type cannot be relied on to indicate the type of the > message (indeed the specification encourages it *not* to be used, relying > only on the type encoding in the AmqpValue section), a different approach is > necessary. > > I've added a method to Message to get the content as a Variant. This can be > used for a whole range of different types of data including binary or maps > and lists. See the changes to qpid-send and qpid-receive for example of how > to use this. > > > Diffs > ----- > > /trunk/qpid/cpp/examples/messaging/drain.cpp 1510678 > /trunk/qpid/cpp/examples/messaging/map_receiver.cpp 1510678 > /trunk/qpid/cpp/examples/messaging/map_sender.cpp 1510678 > /trunk/qpid/cpp/examples/messaging/spout.cpp 1510678 > /trunk/qpid/cpp/include/qpid/messaging/Message.h 1510678 > /trunk/qpid/cpp/src/CMakeLists.txt 1510678 > /trunk/qpid/cpp/src/qpid/amqp/DataBuilder.h PRE-CREATION > /trunk/qpid/cpp/src/qpid/amqp/DataBuilder.cpp PRE-CREATION > /trunk/qpid/cpp/src/qpid/amqp/Encoder.h 1510678 > /trunk/qpid/cpp/src/qpid/amqp/Encoder.cpp 1510678 > /trunk/qpid/cpp/src/qpid/amqp/ListBuilder.h PRE-CREATION > /trunk/qpid/cpp/src/qpid/amqp/ListBuilder.cpp PRE-CREATION > /trunk/qpid/cpp/src/qpid/amqp/MapBuilder.h 1510678 > /trunk/qpid/cpp/src/qpid/amqp/MapBuilder.cpp 1510678 > /trunk/qpid/cpp/src/qpid/amqp/MessageEncoder.h 1510678 > /trunk/qpid/cpp/src/qpid/amqp/MessageEncoder.cpp 1510678 > /trunk/qpid/cpp/src/qpid/amqp/MessageReader.h 1510678 > /trunk/qpid/cpp/src/qpid/amqp/MessageReader.cpp 1510678 > /trunk/qpid/cpp/src/qpid/amqp/descriptors.h 1510678 > /trunk/qpid/cpp/src/qpid/amqp/typecodes.h 1510678 > /trunk/qpid/cpp/src/qpid/broker/amqp/Message.h 1510678 > /trunk/qpid/cpp/src/qpid/broker/amqp/Message.cpp 1510678 > /trunk/qpid/cpp/src/qpid/broker/amqp/Translation.cpp 1510678 > /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.h 1510678 > /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1510678 > /trunk/qpid/cpp/src/qpid/client/amqp0_10/IncomingMessages.cpp 1510678 > /trunk/qpid/cpp/src/qpid/client/amqp0_10/OutgoingMessage.cpp 1510678 > /trunk/qpid/cpp/src/qpid/client/amqp0_10/ReceiverImpl.h 1510678 > /trunk/qpid/cpp/src/qpid/client/amqp0_10/ReceiverImpl.cpp 1510678 > /trunk/qpid/cpp/src/qpid/client/amqp0_10/SessionImpl.cpp 1510678 > /trunk/qpid/cpp/src/qpid/messaging/Message.cpp 1510678 > /trunk/qpid/cpp/src/qpid/messaging/MessageImpl.h 1510678 > /trunk/qpid/cpp/src/qpid/messaging/MessageImpl.cpp 1510678 > /trunk/qpid/cpp/src/qpid/messaging/amqp/EncodedMessage.h 1510678 > /trunk/qpid/cpp/src/qpid/messaging/amqp/EncodedMessage.cpp 1510678 > /trunk/qpid/cpp/src/qpid/messaging/amqp/SenderContext.cpp 1510678 > /trunk/qpid/cpp/src/qpid/types/encodings.h PRE-CREATION > /trunk/qpid/cpp/src/tests/qpid-receive.cpp 1510678 > /trunk/qpid/cpp/src/tests/qpid-send.cpp 1510678 > > Diff: https://reviews.apache.org/r/13328/diff/ > > > Testing > ------- > > > Thanks, > > Gordon Sim > >
