qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alan Conway" <acon...@redhat.com>
Subject Re: Review Request 40794: PROTON-1068: c++ context cleanup
Date Tue, 01 Dec 2015 14:30:12 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message