qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Stitcher" <astitc...@apache.org>
Subject Re: Review Request 40794: PROTON-1068: c++ context cleanup
Date Tue, 01 Dec 2015 15:34:52 GMT


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


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