----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45113/#review124799 -----------------------------------------------------------
Note that the comments here don't imply I like this approach - they just look at what is here. proton-c/bindings/cpp/include/proton/condition.hpp <https://reviews.apache.org/r/45113/#comment187473> Why did you move this down in the definition? It's private here too. proton-c/bindings/cpp/include/proton/condition.hpp (line 37) <https://reviews.apache.org/r/45113/#comment187472> I don't think the default constructor makes sense. It's certainly not currently used anywhere. [This meshes with removing the default argument in close below] proton-c/bindings/cpp/include/proton/condition.hpp (line 69) <https://reviews.apache.org/r/45113/#comment187506> I think "set" is a bit I find especially ugly. Although this concept of condition requuires it. Could we have "friend pn_condition_t* operator=(pn_condition_t*, const condition&)" ? At least that wouldn't be ugly (but would do the same thing). proton-c/bindings/cpp/include/proton/connection.hpp (line 81) <https://reviews.apache.org/r/45113/#comment187476> *This comment applies to session and link* It's important to split this into 2 close methods (For API/ABI reasons: default arguments in the API are bad and should be avoided because they mix the API between the client and the library) one with no arguments and one with the condition argument - one can call the other internally. proton-c/bindings/cpp/include/proton/endpoint.hpp (line 28) <https://reviews.apache.org/r/45113/#comment187477> You can't need both #include here and "class condition" below. proton-c/bindings/cpp/include/proton/endpoint.hpp (line 70) <https://reviews.apache.org/r/45113/#comment187480> Split into 2 virtuals as above. virtual void close(); virtual void close(const condition&); proton-c/bindings/cpp/include/proton/link.hpp (line 69) <https://reviews.apache.org/r/45113/#comment187481> Split as above. proton-c/bindings/cpp/include/proton/session.hpp (line 68) <https://reviews.apache.org/r/45113/#comment187483> Split as above. proton-c/bindings/cpp/include/proton/transport.hpp (line 59) <https://reviews.apache.org/r/45113/#comment187485> Split as above. proton-c/bindings/cpp/src/condition.cpp (line 38) <https://reviews.apache.org/r/45113/#comment187504> As above, this seems to violate the "value"ness of condition. It's really operator=, but that should work anyway because of the conversion constructor. proton-c/bindings/cpp/src/condition.cpp (line 49) <https://reviews.apache.org/r/45113/#comment187505> FWIW (a micro-optimisation) This version is potentially less efficient because it uses it has to construct a bunch of strings, whereas the original version only has to string append. I don't think move helps here. It is a little neater to read though. - Andrew Stitcher On March 21, 2016, 7:40 p.m., Alan Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45113/ > ----------------------------------------------------------- > > (Updated March 21, 2016, 7:40 p.m.) > > > Review request for qpid, Andrew Stitcher, Cliff Jansen, and Justin Ross. > > > Repository: qpid-proton-git > > > Description > ------- > > Allow setting conditions in the "close()" function for endpoints and > transport. > > Modified the proton::condition class as follows: > - normal value semantics, user can construct and copy conditions without > surprises. > - memory safe: no core dumps, no dependency on endpoint lifecycle. > > Example of use: > > // Close a link with an error. > mylink.close(condition("myerr", "something went wrong")); > > // Throw condition "name: description" as a proton::error > if (!session.condition().empty()) > throw proton::error(session.condition().what()); > > // Save the condition to check/report/throw later > condition save_error = connection.condition(); > > > Diffs > ----- > > proton-c/bindings/cpp/include/proton/condition.hpp > 9e157f3c7963530f469d1fb40f52fa12fe57d7bd > proton-c/bindings/cpp/include/proton/connection.hpp > da7f806c745e2c52c4d2677fea669105d8c703ce > proton-c/bindings/cpp/include/proton/endpoint.hpp > d59e09324887c0f6cf22da99816cf0075f8b79b7 > proton-c/bindings/cpp/include/proton/link.hpp > 8de2e7ba0f944e169560591ecbc4b32c182d8dee > proton-c/bindings/cpp/include/proton/session.hpp > 014ab2a2d2b3c180a42162e7b22454492985f61c > proton-c/bindings/cpp/include/proton/transport.hpp > 9e32ac508a6c711b912aaadcb7bea5923e6b47e1 > proton-c/bindings/cpp/src/condition.cpp > b60425130b2116ebba8a08b3fac58fe4aa95d549 > proton-c/bindings/cpp/src/connection.cpp > fc61190e900b9c2e48a98d06149221f03e30cd0a > proton-c/bindings/cpp/src/link.cpp 1ac730442018a4e93fad731941c888e600a68142 > proton-c/bindings/cpp/src/session.cpp > 545869f78ec1183a549f9c396908a530869ef1f6 > proton-c/bindings/cpp/src/transport.cpp > 88838064810e43dfbb2b44f51d87504d2a5d6d75 > > Diff: https://reviews.apache.org/r/45113/diff/ > > > Testing > ------- > > > Thanks, > > Alan Conway > >
