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