> On Aug. 12, 2015, 3:20 p.m., Alan Conway wrote:
> > 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.
Here's some code to show what I mean, I created a new review from tip of
cjansen-cpp-client as otherwise the diffs would quite confusing.
https://reviews.apache.org/r/37408/
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37217/#review95090
-----------------------------------------------------------
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
>
>
