-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37217/#review95090
-----------------------------------------------------------
I want to clarify my thoughts for discussion, I'm working on putting this into
code.
DESIGN PRINCIPLES:
1. C++ is not python. In C++ it is normal to provide "raw" alloc/free style
pairs, esp. when wrapping C. Automation of memory management in C++ is done by
a "smart pointer" layer (e.g. shared_ptr, unique_ptr) that is *separate* from
the underlying raw access.
2. Proton refcounting should be considered an internal implementation detail of
proton, NOT a feature that we use in the C++ binding. If need to refcount in
the binding we should maintain our own refcounts to references *from the
binding*, not rely on pn refcounts.
3. I suggest something like this: for pn_foo we have
a. foo : public wrapper<pn_foo> {} behaves like a plain pn_foo* but has
convenience C++ member functions and "free()" member calling appropriate
pn_foo_free()
b. unique<foo> : behaves like foo but calls foo.free() in dtor
c. shared<foo> : behaves like foo but maintains a refcount (like shared_ptr)
and calls foo.free() on last ref.
4. Objects belonging to the binding itself that are not wrapper<pn_foo> should
have normal new/delete semantics, and be manageable by std::shared_ptr,
std::unique_ptr etc.
Some justification for 2: Consider this from the public proton API doc for
pn_link_target:
* The pointer returned by this operation is valid until the link
* object is freed.
Note the text specificaly says the pointer valid *until the link is freed*. It
does *not* say you can incref the pointer to keep it around longer. It so
happens that right now that actually works but IMO that is an implementation
detail we should not rely on. In a future, simpler implementation of proton as
a more C-like library that is not so heavily influenced by python it might NOT
work. The python bindings rely on proton refcounts because they were
specifically designed to support python, I don't think languages like C++ or Go
should rely on them (and IMO this detail of python and ruby-like languages
should not have been pushed into the C library, but should have been dealt with
in those bindings) Go and C++ which have their own interface with C should
treat proton as a simple C library.
I suspect that simple 3a + 3b will cover most cases, I'm not sure we even need
3c but it isn't that hard to do.
ASIDE: We could do some really freaky C++ magic and make wrapper<pn_foo> look
exactly like a real C++ object with a real pointer (imaginary C++ objects
occupying the same memory location as the pn_foo_*, casting their `this`
pointers with overloaded op delete and all that fun stuff) but I think that
would be, um, silly.
- Alan Conway
On Aug. 7, 2015, 1:46 p.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37217/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 1:46 p.m.)
>
>
> Review request for qpid, Alan Conway and Andrew Stitcher.
>
>
> Bugs: PROTON-865
> https://issues.apache.org/jira/browse/PROTON-865
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> This is a mini refactor of a single class to see if the chosen direction for
> generally removing handle<foo> is sane.
>
>
> I chose the connection class to try based on medium complexity and
> interaction with other moving parts (not visible to the user, the
> connector/override and life cycle issues).
>
> The connection is now non copyable.
>
> If the connection "self-created" (i.e. an inbound connection from a listener,
> arriving via a reactor event, or implicit connection from
> container.create_receiver), it is not "owned", and its life ends when the
> pn_connection_t dies (dies for good, i.e. when it releases its attachment
> records).
>
> If the user creates the connection explicitly, it is "owned" and destructs as
> normal. It is unclear if the Python behavior should be preserved, where the
> connection lives on un-owned, or destruction means "all gone" with tear down
> of the transport and connector. I chose the latter as perhaps least surprise
> for the user.
>
> The Proton C++ code can use either model internally to taste.
>
> Perhaps the connection object should have a pin/unpin or incref/decref
> ability so that the user can hang onto it past the event delivery. As a
> hack, they can always do pn_incref(connection.pn_connection()) and decref it
> when done.
>
> It is not clear to me that the counted class will ever be used separately
> from the context<foo> class, so they might get rolled together in the future.
> The memory management is accomplished by a special proton metaclass that is
> refcounted, but neither allocates nor frees memory.
>
> connection::getZZZcontainer() is an unintended artifact of the refactor. I'm
> not sure why gcc is fussing over the original signature, but I don't wish to
> target its resolution in this review.
>
>
> Diffs
> -----
>
> examples/cpp/helloworld.cpp 34e5076
> examples/cpp/server.cpp 78b78d3
> proton-c/bindings/cpp/CMakeLists.txt d1b1ebc
> proton-c/bindings/cpp/include/proton/connection.hpp af3c585
> proton-c/bindings/cpp/include/proton/container.hpp a0ca59a
> proton-c/bindings/cpp/include/proton/context.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/counted.hpp PRE-CREATION
> proton-c/bindings/cpp/src/blocking_connection.cpp c0c1477
> proton-c/bindings/cpp/src/blocking_connection_impl.hpp 11ff4fd
> proton-c/bindings/cpp/src/blocking_connection_impl.cpp 0e78541
> proton-c/bindings/cpp/src/connection.cpp 2b335a4
> proton-c/bindings/cpp/src/connection_impl.hpp 6450ef5
> proton-c/bindings/cpp/src/connection_impl.cpp e2f4608
> proton-c/bindings/cpp/src/connector.hpp ad373a9
> proton-c/bindings/cpp/src/connector.cpp 559a7fd
> proton-c/bindings/cpp/src/container.cpp a424c0b
> proton-c/bindings/cpp/src/container_impl.hpp 1ce27ee
> proton-c/bindings/cpp/src/container_impl.cpp e328251
> proton-c/bindings/cpp/src/context.cpp PRE-CREATION
> proton-c/bindings/cpp/src/contexts.cpp 98c502b
> proton-c/bindings/cpp/src/counted.cpp PRE-CREATION
> proton-c/bindings/cpp/src/link.cpp 2cef0a2
> proton-c/bindings/cpp/src/proton_event.cpp 46b43ee
> proton-c/bindings/cpp/src/session.cpp 5742f13
>
> Diff: https://reviews.apache.org/r/37217/diff/
>
>
> Testing
> -------
>
> passes ctest on rhel7
>
>
> Thanks,
>
> Cliff Jansen
>
>
