> 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
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. - Andrew ----------------------------------------------------------- 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 > >
