----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50574/#review143960 -----------------------------------------------------------
NAK Sorry, but this just doesn't solve the real requirements - it treats a surface API issue without fixing the underlying implementation problem. It unfortunately leaves the underlying ABI issue. The point of the piece of work is not API (to allow the API user to copy/move their own classes) although that is the important visible piece of this work. The point is to fix the bad ABI and implementation. One very good thing - I'm very much in favour of having the test. proton-c/bindings/cpp/include/proton/messaging_handler.hpp (line 79) <https://reviews.apache.org/r/50574/#comment209898> As this class must have *no* data members it can't need any infrastructure for copying and moving. proton-c/bindings/cpp/include/proton/messaging_handler.hpp (line 180) <https://reviews.apache.org/r/50574/#comment209899> If you've left this member present then this whole piece of work has very limited value. proton-c/bindings/cpp/src/messaging_adapter.hpp (line 62) <https://reviews.apache.org/r/50574/#comment209900> A messaging adapter cannot exist without the delegate so changing it to a pointer is semantically wrong. - Andrew Stitcher On July 28, 2016, 5:09 p.m., Cliff Jansen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50574/ > ----------------------------------------------------------- > > (Updated July 28, 2016, 5:09 p.m.) > > > Review request for qpid and Andrew Stitcher. > > > Bugs: PROTON-1241 > https://issues.apache.org/jira/browse/PROTON-1241 > > > Repository: qpid-proton-git > > > Description > ------- > > Rework messaging_handler to elliminate delete reference to messaging_adapter > from header files. deal with event flow in copy case versus move case. Add > test > > > Diffs > ----- > > proton-c/bindings/cpp/CMakeLists.txt 1561134 > proton-c/bindings/cpp/include/proton/messaging_handler.hpp 8520846 > proton-c/bindings/cpp/src/handler.cpp cda8acb > proton-c/bindings/cpp/src/handler_test.cpp PRE-CREATION > proton-c/bindings/cpp/src/messaging_adapter.hpp 846b466 > proton-c/bindings/cpp/src/messaging_adapter.cpp 37fc4e0 > > Diff: https://reviews.apache.org/r/50574/diff/ > > > Testing > ------- > > ctest > > > Thanks, > > Cliff Jansen > >
