----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40794/#review108394 -----------------------------------------------------------
proton-c/bindings/cpp/include/proton/data.hpp (line 46) <https://reviews.apache.org/r/40794/#comment167846> shouldn't this be renamed (to say "clone") To avoid clashing with object operator=? proton-c/bindings/cpp/include/proton/object.hpp (line 32) <https://reviews.apache.org/r/40794/#comment167849> 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. proton-c/bindings/cpp/include/proton/object.hpp (line 36) <https://reviews.apache.org/r/40794/#comment167852> 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). proton-c/bindings/cpp/include/proton/object.hpp (line 67) <https://reviews.apache.org/r/40794/#comment167847> I removed the bool conversion deliberately - it's too prone to being found as a last resort when the user didn't expect it - this is fixed in 11 with explicit, but in 03 I think it's a bad idea. proton-c/bindings/cpp/include/proton/object.hpp (line 68) <https://reviews.apache.org/r/40794/#comment167858> 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). proton-c/bindings/cpp/src/contexts.hpp (line 44) <https://reviews.apache.org/r/40794/#comment167860> Since these are both pointers shouldn't the initialisers be (0) or (nullptr)? At least it would be clearer to me. proton-c/bindings/cpp/src/contexts.cpp (line 39) <https://reviews.apache.org/r/40794/#comment167865> I think this is misconceived. You don't need to have the same class with different names. In fact I think this is identical to one of the classes provided in proton-c. proton-c/bindings/cpp/src/contexts.cpp (line 40) <https://reviews.apache.org/r/40794/#comment167867> 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. - Andrew Stitcher 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 > >
