> 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. > > Alan Conway wrote: > 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.
I can't do it. I don't understand how a public base class, with public functions, all intended to be called by the user, is "internal" or "detail" in any sense. This is normal is-a inheritance. C++ doesn't have type restrictions (we can't say `message_id : restriction_of scalar` and remove functions we don't want) so a common base class is the only way to do it. Why would we want to hide that commonality in internal:: or detail:: ? - 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 > >
