> On Aug. 23, 2016, 4:50 p.m., Andrew Stitcher wrote: > > I'm not 100% convinced that moving scalar_base out of internal is justified > > by the inability to get doxygen to generate the correct docs. Can't we fix > > doxygen somehow? > > > > Why scalar_base but not value_base? they have the same function in the API > > structure. > > > > I think it may be correct to actually pull in avarything from the base > > classes with a using directive anyway to give the correct name lookup > > semantics (it matters if you have overloading). > > Alan Conway wrote: > value_base doesn't have the same function, it's solving a template type > recursion problem and there was no need for it to have any public functions > so I just moved them to value. > scalar_base is providing common functionality for scalar, messsage_id > etc. and is used by the encoder and decoder to do the common encoding for > them all. > > I dithered myself. I decided not to "fix" doxygen because doxygen is > accurately reflecting our code conventions: If namespace internal means > "don't touch that" then having public API functions inherited from an > internal base class is a mixed message at best. Other options, which I can > implement if you prefer: > 1. make a separate public proton::scalar_base with just those two > functions that delegates to the internal one > 2. drop the inheritance and duplicate the public functions on all the > derived classes, delegate to an internal impl object. > > 2. is cleanest, 1. less work. I'll bite the bullet and do 2. if you feel > that's the right thing > > Andrew Stitcher wrote: > I don't think internal exactly means "don't touch that", rather you'll > never get this directly or you'll never deal with this object only one of > it's children or if you get one of these don't look inside just rely on it's > interface. > > So it's not that they don't provide API, but that the API is thought of > as attaching to the child - that is why I suggested using "using" as that > gives the expected namespace lookup if there are (accidentally) overloads > with the same name in parent and child. > > So I'd disagree about doxygen respecting our code conventions, but I'm > beginning to suspect that we think "internal" means different things.
To me, the public API is everything you have to document so the user can use the API. Internal is everything else. It's not just doxygen, someone trying to read the headers will most likely never follow the internal::scalar_base inheritance to discover the hidden public API because we called it "internal::scalar_base". So we need something in a public header that makes it clear that this is public stuff, but I can have a go at how doxygen documents using statements or just add an inline function in each subclass. > 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. 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. - Alan ----------------------------------------------------------- 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 > >
