> On Nov. 30, 2015, 9:37 p.m., Andrew Stitcher wrote: > > proton-c/bindings/cpp/include/proton/object.hpp, line 52 > > <https://reviews.apache.org/r/40794/diff/2/?file=1149250#file1149250line52> > > > > I didn't use incref/decref in the headers because I didn't want to > > bring in object.h. I'm not sure that there is any gain to doing this here > > except that you have to because you;ve made it a templated class which as I > > said above is wrong (IMO). > > Alan Conway wrote: > The gain is (at least) two less un-inlinable function calls on every > invocation of every API function that returns an object. Probably not a huge > deal in the overall proton scheme of things, but why add extra overhead for > nothing? What does it add to have wrappers that simply forward the call?
The inlining is something, but it does make the C++ headers dependent on the C headers which they weren't before - I'm not sure if this is hugely significant. > On Nov. 30, 2015, 9:37 p.m., Andrew Stitcher wrote: > > proton-c/bindings/cpp/include/proton/object.hpp, line 32 > > <https://reviews.apache.org/r/40794/diff/2/?file=1149250#file1149250line32> > > > > No, no, no, the entire point of the previous object_base class was > > *not* to be templated and to use void* directly. This seems even more > > apposite if you are explicitly calling it a wrapper of pn pointers. > > Alan Conway wrote: > I don't get it. Why is it better to have a type unsafe `void*` in the C++ > API than to use only type safe smart pointers? Sure its an internal class, > but what does it add execpt the ability to do nonsensical things like `link = > object_base(new int)`? I know that pn_inc/decref take `void*` but thats a > type-unsafe limitation of C, not something we should promote to C++ IMO. Well you cut down hugely on the amouint of template instantiations, whilst still allowing the c++ code to be safe because it uses them. object_base is purely internal and only supposed to be used via the object<T> classes wrapping it. What you did latterly with pn_ptr<> is just not my design, although it looks similar. > On Nov. 30, 2015, 9:37 p.m., Andrew Stitcher wrote: > > proton-c/bindings/cpp/include/proton/object.hpp, line 87 > > <https://reviews.apache.org/r/40794/diff/2/?file=1149250#file1149250line87> > > > > As a principle you should make binary operators like this external > > because otherwise conversion operators can't work on the first argument. > > > > which is why it was this way before. > > > > And that is why I don't think comparable<> is very useful (and didn't > > use it). > > Alan Conway wrote: > Law of least surprise. It is surprising that == works and != doesn't. It > is surprising that < works and > doesn't. A simple comparable template makes > it trivial to add the full set of operators with no extra effort so why > wouldn't you? I don't really agree with this - Objects are generally equality comparable - "is this the same object?", but really ordering them is not generally meaningful - it just happens you can impose an arbitrary order on them by hashing them and comparing the hashes (but this is only useful in specific cases). > On Nov. 30, 2015, 9:37 p.m., Andrew Stitcher wrote: > > proton-c/bindings/cpp/src/contexts.cpp, line 40 > > <https://reviews.apache.org/r/40794/diff/2/?file=1149257#file1149257line40> > > > > I'm *very* wary of this approach *all* the code up to now has used > > statically (in fact constant) allocated pn_class_t structs. This makes > > sense as there are obviously only a finite (and smallish) number of > > possible object types, and they are known at compile time. > > > > Allocating these dynamically doesn't really seem necessary overall, > > escpecially as there are only going to be about 2 of them. > > Alan Conway wrote: > They are allocated statically, but using a template so that we can > in-place allocate and destroy a C++ object inside a proton object without a > double allocation. > There is only one now but there will be more I am certain of it. The > boilerplate to create a pn_class is so hideous and error-prone that I > strongly feel it needs to be centralized. > > The alternative is a single proton class that calls a single generic > delete function for a separately allocated c++ context object, which means we > are forced to do two allocations for every context. > > Andrew Stitcher wrote: > How are they static? the pn_class_t is constructed on the stack and then > copied out etc. *ALL* of the class structs in the rest of the code are > *compile time* constants and hence go in read-only (well relocatable ro) > sections. > > I agree the boilerplate is hard to understand - and that is exactly what > I I'm loking at presently (well except for reviewing this change, which seems > to be taking more time currently). > > Alan Conway wrote: > The pn_factory instance is a global variable, the fact that a temporary > is used to initialize its pn_class_t has no effect on its scope or extent. > > Alan Conway wrote: > You've inspired me - better solution coming, single statically allocate > pn_class_t, context base class with virtual destructor, so we can have a > single PN class that holds arbitrary C++ payloads with proper mem mgmt. I think you are using the word static to mean something else (until perhaps the last comment here). - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40794/#review108394 ----------------------------------------------------------- On Nov. 30, 2015, 8:23 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, 8:23 p.m.) > > > Review request for qpid, Andrew Stitcher and Cliff Jansen. > > > Repository: qpid-proton-git > > > Description > ------- > > - 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. > > PROTON-1068: c++ rename data::op== and < to avoid clash with object<> ops. > > > PROTON-1068: Restore protected object, introduce pn_ptr for internal > refcounts. > > Introduce internal pn_ptr for internal refcounted pointer use. > Simplify & restore safety of object<> base class. > > > 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/decoder.cpp > f14345db20f0d7d00572211cff4eed52a4d8c2a8 > proton-c/bindings/cpp/src/encoder.cpp > a0b10c017ca94981b7538fc2f60643c4e35d7f6a > proton-c/bindings/cpp/src/link.cpp 587dec92e7fe0fa40696b66dcffb4b74b9c1b4d0 > proton-c/bindings/cpp/src/messaging_adapter.cpp > e3bced871c3836ce3c58a0cece7bea3c223e5a87 > proton-c/bindings/cpp/src/object.cpp > d5fb7ef95cc8c29b74897cbd9ec8173a61c8a9af > proton-c/bindings/cpp/src/reactor.cpp > 9a52d995b5bdbf393ed1dc49c69903f3f526516e > proton-c/bindings/cpp/src/session.cpp > e9030804b92034705b74945d1ff92a1239bec4a3 > proton-c/bindings/cpp/src/value.cpp > be5ca5edfc60dfa523d6c00cd2a551942612e22a > > Diff: https://reviews.apache.org/r/40794/diff/ > > > Testing > ------- > > ctest -R cpp with valgrind, no leaks. > > > Thanks, > > Alan Conway > >