You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@qpid.apache.org by Alan Conway <ac...@redhat.com> on 2015/11/30 21:23:02 UTC

Re: Review Request 40794: PROTON-1068: c++ context cleanup

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


Changes
-------

Restored privacy to object<> internals, re-introduced refcounted pn_ptr for internal use.


Summary (updated)
-----------------

PROTON-1068: c++ context cleanup


Repository: qpid-proton-git


Description (updated)
-------

- 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 (updated)
-----

  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

Re: Review Request 40794: PROTON-1068: c++ context cleanup

Posted by Alan Conway <ac...@redhat.com>.


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

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


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

Re: Review Request 40794: PROTON-1068: c++ context cleanup

Posted by Andrew Stitcher <as...@apache.org>.


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

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


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

Re: Review Request 40794: PROTON-1068: c++ context cleanup

Posted by Alan Conway <ac...@redhat.com>.


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

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?


- Alan


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

Re: Review Request 40794: PROTON-1068: c++ context cleanup

Posted by Andrew Stitcher <as...@apache.org>.


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

Re: Review Request 40794: PROTON-1068: c++ context cleanup

Posted by Alan Conway <ac...@redhat.com>.


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

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?


- Alan


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

Re: Review Request 40794: PROTON-1068: c++ context cleanup

Posted by Alan Conway <ac...@redhat.com>.


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

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.


- Alan


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

Re: Review Request 40794: PROTON-1068: c++ context cleanup

Posted by Alan Conway <ac...@redhat.com>.


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

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.


- Alan


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

Re: Review Request 40794: PROTON-1068: c++ context cleanup

Posted by Alan Conway <ac...@redhat.com>.


> On Nov. 30, 2015, 9:37 p.m., Andrew Stitcher wrote:
> > proton-c/bindings/cpp/include/proton/data.hpp, line 47
> > <https://reviews.apache.org/r/40794/diff/2/?file=1149248#file1149248line47>
> >
> >     shouldn't this be renamed (to say "clone") To avoid clashing with object operator=?

Yes.


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

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.


- Alan


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

Re: Review Request 40794: PROTON-1068: c++ context cleanup

Posted by Alan Conway <ac...@redhat.com>.


> 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.
> 
> Andrew Stitcher wrote:
>     I think you are using the word static to mean something else (until perhaps the last comment here).

Probably you mean POD initialized and I was talking about global C++ constructors, but anyway see the latest diff, it is really static, POD initialized now :)


- Alan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40794/#review108394
-----------------------------------------------------------


On Dec. 1, 2015, 4:07 p.m., Alan Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40794/
> -----------------------------------------------------------
> 
> (Updated Dec. 1, 2015, 4:07 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.
> 
> PROTON-1068: Fix astitcher issues raised on reviewboard https://reviews.apache.org/r/4079
> 
> Context classes:
> - single context base class with a single pn_class_t declared using normal C macros.
> - arbitrary C++ payload type can be created and destructed correctly wihtout  an extra allocation.
> - renamed data::opeartor= to copy to avoid clashs with object<>::op= (it is a data copy not a clone)
> - removed operator bool, made op == and < operators friends
> 
> 
> 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/include/proton/value.hpp 661de51f8a0c75f05d5c76f4bd0385f911a8ec60 
>   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/message.cpp 7c032f01e828ac36ff47054c42164953897fe8e0 
>   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
> 
>

Re: Review Request 40794: PROTON-1068: c++ context cleanup

Posted by Andrew Stitcher <as...@apache.org>.

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

Re: Review Request 40794: PROTON-1068: c++ context cleanup

Posted by Andrew Stitcher <as...@apache.org>.


> On Dec. 4, 2015, 4:24 p.m., Andrew Stitcher wrote:
> > Not quite what I had in mind, but good enough.
> 
> Alan Conway wrote:
>     As long as the public API is OK, you can hack on the internals all you want :)
>     I think our main differences is keeping pn_ptr for internal use. At this point it is only used for handlers, if you want to give them the full object treatment and get rid of pn_ptr I have no objection.

The problem with handlers is that they have a strange inheritance hierarchy - with vector<Handler> or similar at the top - that makes it fiddly to turn them into objects - I wanted to but backed away.

BTW I think they will be more uses of pn_ptr after you merge/rebase with current master.


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40794/#review108991
-----------------------------------------------------------


On Dec. 2, 2015, 2:30 p.m., Alan Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40794/
> -----------------------------------------------------------
> 
> (Updated Dec. 2, 2015, 2:30 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.
> 
> PROTON-1068: Fix astitcher issues raised on reviewboard https://reviews.apache.org/r/4079
> 
> Context classes:
> - single context base class with a single pn_class_t declared using normal C macros.
> - arbitrary C++ payload type can be created and destructed correctly wihtout  an extra allocation.
> - renamed data::opeartor= to copy to avoid clashs with object<>::op= (it is a data copy not a clone)
> - removed operator bool, made op == and < operators friends
> 
> PROTON-1068: Fix final astitcher issues raised on reviewboard https://reviews.apache.org/r/4079
> 
> 
> 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/include/proton/value.hpp 661de51f8a0c75f05d5c76f4bd0385f911a8ec60 
>   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/message.cpp 7c032f01e828ac36ff47054c42164953897fe8e0 
>   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
> 
>

Re: Review Request 40794: PROTON-1068: c++ context cleanup

Posted by Alan Conway <ac...@redhat.com>.


> On Dec. 4, 2015, 4:24 p.m., Andrew Stitcher wrote:
> > Not quite what I had in mind, but good enough.

As long as the public API is OK, you can hack on the internals all you want :)
I think our main differences is keeping pn_ptr for internal use. At this point it is only used for handlers, if you want to give them the full object treatment and get rid of pn_ptr I have no objection.


- Alan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40794/#review108991
-----------------------------------------------------------


On Dec. 2, 2015, 2:30 p.m., Alan Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40794/
> -----------------------------------------------------------
> 
> (Updated Dec. 2, 2015, 2:30 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.
> 
> PROTON-1068: Fix astitcher issues raised on reviewboard https://reviews.apache.org/r/4079
> 
> Context classes:
> - single context base class with a single pn_class_t declared using normal C macros.
> - arbitrary C++ payload type can be created and destructed correctly wihtout  an extra allocation.
> - renamed data::opeartor= to copy to avoid clashs with object<>::op= (it is a data copy not a clone)
> - removed operator bool, made op == and < operators friends
> 
> PROTON-1068: Fix final astitcher issues raised on reviewboard https://reviews.apache.org/r/4079
> 
> 
> 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/include/proton/value.hpp 661de51f8a0c75f05d5c76f4bd0385f911a8ec60 
>   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/message.cpp 7c032f01e828ac36ff47054c42164953897fe8e0 
>   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
> 
>

Re: Review Request 40794: PROTON-1068: c++ context cleanup

Posted by Andrew Stitcher <as...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40794/#review108991
-----------------------------------------------------------

Ship it!


Not quite what I had in mind, but good enough.

- Andrew Stitcher


On Dec. 2, 2015, 2:30 p.m., Alan Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40794/
> -----------------------------------------------------------
> 
> (Updated Dec. 2, 2015, 2:30 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.
> 
> PROTON-1068: Fix astitcher issues raised on reviewboard https://reviews.apache.org/r/4079
> 
> Context classes:
> - single context base class with a single pn_class_t declared using normal C macros.
> - arbitrary C++ payload type can be created and destructed correctly wihtout  an extra allocation.
> - renamed data::opeartor= to copy to avoid clashs with object<>::op= (it is a data copy not a clone)
> - removed operator bool, made op == and < operators friends
> 
> PROTON-1068: Fix final astitcher issues raised on reviewboard https://reviews.apache.org/r/4079
> 
> 
> 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/include/proton/value.hpp 661de51f8a0c75f05d5c76f4bd0385f911a8ec60 
>   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/message.cpp 7c032f01e828ac36ff47054c42164953897fe8e0 
>   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
> 
>

Re: Review Request 40794: PROTON-1068: c++ context cleanup

Posted by Alan Conway <ac...@redhat.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40794/
-----------------------------------------------------------

(Updated Dec. 2, 2015, 2:30 p.m.)


Review request for qpid, Andrew Stitcher and Cliff Jansen.


Changes
-------

Fixed last remaining objections - `void*` base for template and dropped inclusion of object.h


Repository: qpid-proton-git


Description (updated)
-------

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

PROTON-1068: Fix astitcher issues raised on reviewboard https://reviews.apache.org/r/4079

Context classes:
- single context base class with a single pn_class_t declared using normal C macros.
- arbitrary C++ payload type can be created and destructed correctly wihtout  an extra allocation.
- renamed data::opeartor= to copy to avoid clashs with object<>::op= (it is a data copy not a clone)
- removed operator bool, made op == and < operators friends

PROTON-1068: Fix final astitcher issues raised on reviewboard https://reviews.apache.org/r/4079


Diffs (updated)
-----

  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/include/proton/value.hpp 661de51f8a0c75f05d5c76f4bd0385f911a8ec60 
  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/message.cpp 7c032f01e828ac36ff47054c42164953897fe8e0 
  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

Re: Review Request 40794: PROTON-1068: c++ context cleanup

Posted by Alan Conway <ac...@redhat.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40794/
-----------------------------------------------------------

(Updated Dec. 1, 2015, 4:07 p.m.)


Review request for qpid, Andrew Stitcher and Cliff Jansen.


Changes
-------

Fixed most of Andrews issues

    - single context base class with a single pn_class_t declared using normal C macros.
    - arbitrary C++ payload type can be created and destructed correctly wihtout  an extra allocation.
    - renamed data::opeartor= to copy to avoid clashs with object<>::op= (it is a data copy not a clone)
    - removed operator bool, made op == and < operators friends


Repository: qpid-proton-git


Description (updated)
-------

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

PROTON-1068: Fix astitcher issues raised on reviewboard https://reviews.apache.org/r/4079

Context classes:
- single context base class with a single pn_class_t declared using normal C macros.
- arbitrary C++ payload type can be created and destructed correctly wihtout  an extra allocation.
- renamed data::opeartor= to copy to avoid clashs with object<>::op= (it is a data copy not a clone)
- removed operator bool, made op == and < operators friends


Diffs (updated)
-----

  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/include/proton/value.hpp 661de51f8a0c75f05d5c76f4bd0385f911a8ec60 
  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/message.cpp 7c032f01e828ac36ff47054c42164953897fe8e0 
  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