You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Alan Conway <ac...@redhat.com> on 2016/03/21 18:45:01 UTC
Review Request 45113: NO-JIRA: c++: Allow setting of conditions on
endpoints.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45113/
-----------------------------------------------------------
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
Re: Review Request 45113: NO-JIRA: c++: Allow setting of conditions on
endpoints.
Posted by Alan Conway <ac...@redhat.com>.
> On March 22, 2016, 4:07 p.m., Andrew Stitcher wrote:
> > Note that the comments here don't imply I like this approach - they just look at what is here.
OK, I want to focus on the connection_engine so I'll let you tackle this as you would like. I think either approach is fine.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45113/#review124799
-----------------------------------------------------------
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
>
>
Re: Review Request 45113: NO-JIRA: c++: Allow setting of conditions on
endpoints.
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 45113: NO-JIRA: c++: Allow setting of conditions on
endpoints.
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
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.
Changes
-------
Fix typo.
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 (updated)
-----
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