qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Stitcher <astitc...@apache.org>
Subject Re: Review Request 45113: NO-JIRA: c++: Allow setting of conditions on endpoints.
Date Tue, 22 Mar 2016 16:07:51 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message