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