> On Nov. 30, 2015, 4:15 p.m., Andrew Stitcher wrote: > > I'm happy with much of this: > > > > On reflection I think owned_object is redundant given the take_ownership > > flag of the object constructor from a C object. > > > > I do prefer keeping the contexts out of the visible C++ API (but see below). > > > > The big thing I don't like here is the use of the code in > > object_internal.hpp to completely sidestep pn_object() being protected. > > > > Making pn_object() protected was the driving force behind improving the > > code's layering - it forced access to a C object to go through the C++ > > wrapper - this I think actually allows you to know reasonably certainly how > > the layering works (and know which C APIs are being used). Allowing any > > class in the wrappers to access anything in the C API is a mistake in my > > opinion. > > > > It seems to me that this may be necessary because of removing contexts from > > the C++ API (although I may be mistaken) in that case I think that this > > aspect is deferable. > > > > The direction I was moving in (although didn't actually do) was moving > > access to contexts into object and object_base - ultimately this is the > > correct place for it (IMO) because that is how the underlying C object > > system works too. So I feel this aspect of these changes has just reverted > > to the previous status quo which I was trying to move away from.
It was partly because I re-used object<> as an internal refcounted pointer for contexts, partly because we sometimes need to flip between the API object and the internal C object in the impl, and that may be across classes. Let me take a shot at separating out those concerns.... - Alan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40794/#review108318 ----------------------------------------------------------- On Nov. 30, 2015, 2:53 p.m., Alan Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40794/ > ----------------------------------------------------------- > > (Updated Nov. 30, 2015, 2:53 p.m.) > > > Review request for qpid, Andrew Stitcher and Cliff Jansen. > > > Repository: qpid-proton-git > > > Description > ------- > > Strictly a refactor to simplify things and prune dead code, no > API/functionality changes. > Individual commits at > https://github.com/alanconway/qpid-proton/tree/cpp-memory-cleanup > > > PROTON-1068: c++ context cleanup > - got rid of proton::counted base class > - use direct, native pn refcounts for connection contexts. > - remove mention of internal context types and functions from public headers. > > PROTON-1068: c++: Remove owned_object > > owned_object<> is actually exactly equivalent to the take_ownership > constructor > of object<> combined with the C++11 move constructor. Since it behaves > correctly > in C++03 also (with only the added cost of a single inc/dec ref) and is only > used in one place, it seems better to remove the extra complexity. > > PROTON-1068: c++: Remove counted_ptr > > Replace all use of counted_ptr with object<> for consistent memory management. > > QUESTIONS: > > - Drop object_base and fold it into object template? It doesn't do much. > - Call pn_inc/decref direct from object<>? The separate static functions > don't add much. > - Make pn_object() public (with dire warnings) to allow mixed C/C++ > programming? > - could be a separate free function like the one in object_internal to > emphasize the risk. > > > Diffs > ----- > > proton-c/bindings/cpp/CMakeLists.txt > 47ca937991f4f6ee8b6aa947772acbd1f6d8022e > proton-c/bindings/cpp/include/proton/blocking_link.hpp > 2b0a089e8990100add0bbc043b40bda5fc6b958c > proton-c/bindings/cpp/include/proton/connection.hpp > cc8c5ba6f5616dc1062ae7c137f4591f7dcef6e6 > proton-c/bindings/cpp/include/proton/counted.hpp > 2195d93da85a4d61428449f79f2006b2203ef222 > proton-c/bindings/cpp/include/proton/counted_ptr.hpp > 0865e4ace6d2534f7f6ecc0f1c1a1899f4a09db7 > proton-c/bindings/cpp/include/proton/data.hpp > e74b77b92cb00750dc22dd02a90123c7cdc98c67 > proton-c/bindings/cpp/include/proton/handler.hpp > 70da48b217faade1262698d274e42e61f06c5939 > proton-c/bindings/cpp/include/proton/object.hpp > 118f40d15e97d3ea665d916d198ebf4177344ea6 > proton-c/bindings/cpp/include/proton/reactor.hpp > 5f628ce0d3619746fa546e8e10773ee5c1d417e6 > proton-c/bindings/cpp/src/connection.cpp > 733facebd630803d14160c73bafcbca13aa293b7 > proton-c/bindings/cpp/src/connection_options.cpp > 6d5b8297c470b0ad4d0188115bba640a8ac249fa > proton-c/bindings/cpp/src/container_impl.hpp > d9401d09a5e8b4efe87a697e10e1d9a7979b4d0f > proton-c/bindings/cpp/src/container_impl.cpp > bb4f4c5ab5c2388297a460746e0b736e98c21665 > proton-c/bindings/cpp/src/contexts.hpp > a0e6cf6f4615aa681c5ad7931e79a5ec7f856642 > proton-c/bindings/cpp/src/contexts.cpp > 4d17e545976d4a2672006f4288b4f66d2062be7a > proton-c/bindings/cpp/src/counted_ptr.cpp > 261d5ec0205b44b90aeebbac901e856f5c7344a2 > proton-c/bindings/cpp/src/data.cpp 8a45d6bf0febb602de40d96c727c1a95cf002171 > proton-c/bindings/cpp/src/link.cpp 587dec92e7fe0fa40696b66dcffb4b74b9c1b4d0 > proton-c/bindings/cpp/src/reactor.cpp > 9a52d995b5bdbf393ed1dc49c69903f3f526516e > proton-c/bindings/cpp/src/session.cpp > e9030804b92034705b74945d1ff92a1239bec4a3 > > Diff: https://reviews.apache.org/r/40794/diff/ > > > Testing > ------- > > ctest -R cpp with valgrind, no leaks. > > > Thanks, > > Alan Conway > >
