> On Aug. 23, 2016, 4:50 p.m., Andrew Stitcher wrote: > > proton-c/bindings/cpp/include/proton/message_id.hpp, line 45 > > <https://reviews.apache.org/r/51336/diff/2/?file=1482124#file1482124line45> > > > > This is an actual API change (even if it is a correct one). > > > > Can message_id really be empty? > > Alan Conway wrote: > No, but it can be missing from the message properties, which is different > from being present with 0 or "". If it is missing there's no safe/sensible > way to choose a default "empty" value - should it be an empty string or > binary, or a 0 integer? Also it is quite valid not to set a correlation_id, > which is semantically different from a correlation-id of 0 or "", so we do > need to represent "missing value" somehow. > > Andrew Stitcher wrote: > Ok, I think you actually answered that message_id *can* be empty! It's > empty if correlation_id is not set at all. > > In which case this is a bug fix. > > Alan Conway wrote: > Well technically: message_id can only be one of 4 types, NULL is not one > of them. So a message_id cannot be empty, which is why I didn't have it there > originally. However it can be *missing* from a message, so adding the "empty" > possibility allows us to model that in a natural way which is similar to how > C, python etc. model it with empty pn_data_t, NULL, None etc. > > Andrew Stitcher wrote: > Now we're really getting to quibles! > > I'd say that a correlation_id can't be null (but there might not be one > at all) but the type modelling it message_id can be null to model that it > isn't present! > > Alan Conway wrote: > Now we are just agreeing in an argumentative way.
Just to join in the pedantry, both message_id and correlation_id *can* be null. The properties section is a list, and a null entry in that list is perfectly valid (indeed would be required e.g. if you didn't want to provide a correlation-id value but did want to provide a content-type). - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51336/#review146545 ----------------------------------------------------------- On Aug. 24, 2016, 3:33 p.m., Alan Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51336/ > ----------------------------------------------------------- > > (Updated Aug. 24, 2016, 3:33 p.m.) > > > Review request for qpid and Andrew Stitcher. > > > Repository: qpid-proton-git > > > Description > ------- > > Added empty() to scalar_base() > > Moved internal::scalar_base to proton::scalar_base. All it's public member > functions are part of the public API for proton::scalar, message_id and > annotation_key > so putting the class that contains them in the namespace internal makes that > unclear > and prevents doxygen from documenting them. > > Moved internal::value_base::empty() and type() to proton::value. There's no > need for them > to be on the internal class and they are public API. > > Note all scalar types need empty() because although the restricted types are > only allowed to be set as specific typesm, they can be missing from a message > which is represented by empty(). > > > Diffs > ----- > > proton-c/bindings/cpp/include/proton/annotation_key.hpp > c0c8ca6cd4d09a5d1720dd2ac8a16a71f91ab8b3 > proton-c/bindings/cpp/include/proton/codec/encoder.hpp > 847e0a9f29aba973f96fa06e3268a1bbe063cd53 > proton-c/bindings/cpp/include/proton/internal/scalar_base.hpp > 2e491d193d604d52b7ec91af510770fc9b4ac66e > proton-c/bindings/cpp/include/proton/message.hpp > 3f683f655346293ab6c043c56d9caed47a391b85 > proton-c/bindings/cpp/include/proton/message_id.hpp > 84f8ab0c28d5afa39e19a78040c0e07fd96b19b9 > proton-c/bindings/cpp/include/proton/scalar.hpp > ab7cb2a514d697c365cf568b4be7a842ae29aa3e > proton-c/bindings/cpp/include/proton/value.hpp > e7e9f7933038c1ba23b5afdbb102d59208e9ea4c > proton-c/bindings/cpp/src/encoder.cpp > e718c402a46d5423b70d178c1903b6ad767eeec5 > proton-c/bindings/cpp/src/message.cpp > ca5f2d5e010f4ffe4bb8bf8b39ed84b6d25db945 > proton-c/bindings/cpp/src/message_test.cpp > 68205c9a04fe6e1edb2000bcc9091ef02748878f > proton-c/bindings/cpp/src/scalar_base.cpp > c005122dedcfabe135bb62fe6c16eafe4da630cb > proton-c/bindings/cpp/src/value.cpp > c2232431f9f093e3530342ea5e3680a62737a4c8 > > Diff: https://reviews.apache.org/r/51336/diff/ > > > Testing > ------- > > ctest -R cpp on f23 clang/gcc 03/11, appveyor, travis > > > Thanks, > > Alan Conway > >
