Re: Review Request 38859: Application events: C++ binding

[Alan Conway <ac...@redhat.com>]

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



examples/cpp/application_events.cpp (line 31)
<https://reviews.apache.org/r/38859/#comment158888>

    Suggest 2 bases:
    
        application_event
        counted_application_event : public application_event, public counted
        
    This will make it easier to write a safe `push()` API since we can specify that a type must be *both* counted *and* an application event.



examples/cpp/application_events.cpp (line 46)
<https://reviews.apache.org/r/38859/#comment158887>

    Use `proton::counted_ptr` to pass counted events. See changes to push()



proton-c/bindings/cpp/include/proton/application_event.hpp (line 41)
<https://reviews.apache.org/r/38859/#comment158882>

    Instead of constructors, provide setters, that way application can set (or not) any collection of event attributes that it wants to.



proton-c/bindings/cpp/include/proton/application_event.hpp (line 50)
<https://reviews.apache.org/r/38859/#comment158881>

    Don't need getters, we inherit from event which already provides them.



proton-c/bindings/cpp/include/proton/application_event.hpp (line 64)
<https://reviews.apache.org/r/38859/#comment158883>

    Suggest we provide a single way to extend events: either inheritance, *or* a data pointer not both. `void*` is nasty I would  go with inheritance only. The user can write their own derived class with a `void*` data pointer if they like that sort of thing.



proton-c/bindings/cpp/include/proton/container.hpp (line 90)
<https://reviews.apache.org/r/38859/#comment158889>

    Suggest two overloads to make memory management clear. This avoids need for casting and is more type safe:
    
        push(application_event &e)
        push(const counted_ptr<counted_application_event>& e)



proton-c/bindings/cpp/src/container_impl.cpp (line 212)
<https://reviews.apache.org/r/38859/#comment158885>

    Two versions of push() means no need for casting.


- Alan Conway


On Sept. 29, 2015, 7:27 p.m., Cliff Jansen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38859/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2015, 7:27 p.m.)
> 
> 
> Review request for qpid, Alan Conway, Andrew Stitcher, Gordon Sim, and Justin Ross.
> 
> 
> Bugs: PROTON-865
>     https://issues.apache.org/jira/browse/PROTON-865
> 
> 
> Repository: qpid-proton-git
> 
> 
> Description
> -------
> 
> This implementation follows the Python version fairly closely except that on_foo() method names for the call backs cannot be embedded in the C++ version.
> 
> A single ApplicationEvent can correspond to 0..N outstanding triggered proton C events.
> 
> An ApplicationEvent will always be dispatched to the main/default handler (same as Python).  Even if assigned a "context" for connection/link/etc.  Perhaps the event callback should flow to the context's handler as for other events, or a separate handler specifier should be added to push/push_event.  Or proton::handler should be ignored completely and an arbitrary std::function callback (or equivalent for pre-c++11) should be considered (already available in derived subclasses).
> 
> A C++ ApplicationEvent can have a void * data pointer associated with it.  It can also be subclassed to include arbitrarily complex additional data or behavior.
> 
> The application is responsible for keeping the ApplicationEvent alive until the last pending event has been processed by all handlers and child handlers.  This is non-trivial in the case of an event that should be discarded right after use.  To assist, if the custom event is derived from proton::counted, the counted refcount will be incremented for each push and decremented each time the Proton C reactor finishes dispatching the event everywhere.
> 
> Bugs:  In addition to being restricted to a single handler at the moment, two pushes of the same event in succession result in the second event being lost (also happens in Python, due to pn_collector_put() eliminating a second of two events it sees as identical).  This can be worked around by defining and pushing an always-ignored event between the two, or define a new pn_collector_put_but_duplicates_ok proton C function.
> 
> 
> Diffs
> -----
> 
>   examples/cpp/CMakeLists.txt 1ac1b1e 
>   examples/cpp/application_events.cpp PRE-CREATION 
>   proton-c/bindings/cpp/CMakeLists.txt 868bbb9 
>   proton-c/bindings/cpp/include/proton/application_event.hpp PRE-CREATION 
>   proton-c/bindings/cpp/include/proton/container.hpp 98754cf 
>   proton-c/bindings/cpp/include/proton/messaging_handler.hpp af5d78b 
>   proton-c/bindings/cpp/src/application_event.cpp PRE-CREATION 
>   proton-c/bindings/cpp/src/application_event_impl.hpp PRE-CREATION 
>   proton-c/bindings/cpp/src/container.cpp f9dc06e 
>   proton-c/bindings/cpp/src/container_impl.hpp 6210b23 
>   proton-c/bindings/cpp/src/container_impl.cpp 3aab33f 
>   proton-c/bindings/cpp/src/messaging_handler.cpp d8b0262 
> 
> Diff: https://reviews.apache.org/r/38859/diff/
> 
> 
> Testing
> -------
> 
> linux so far.  Python binding to verify behavior there too.
> 
> 
> Thanks,
> 
> Cliff Jansen
> 
>