qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alan Conway" <acon...@redhat.com>
Subject Re: Review Request 39222: C++ binding link options
Date Thu, 15 Oct 2015 13:47:06 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39222/#review102276
-----------------------------------------------------------



proton-c/bindings/cpp/include/proton/connection.hpp (line 71)
<https://reviews.apache.org/r/39222/#comment159854>

    I agree addr and handler are special and deserve to be direct arguments, but I think "dynamic"
should be just another link option. It's not the most common use case.



proton-c/bindings/cpp/include/proton/link_options.hpp (line 45)
<https://reviews.apache.org/r/39222/#comment159853>

    using `link_option*` introduces a memory management headache. I'd prefer options to be
copyable value types. There's a big C++11/C++03 issue here. C++11 has vector initializers
and function closures which would make all this much neater, but this is pretty basic functionality
to make available only in C++11. Not sure what the best solution is.
    
    link_options should itself be public : link_option. That gives you more flexiblity about
composing them - you could create a couple of "canned" link_options that set common things
for your app, and then add them to a link_options with specific settings for a particular
link.



proton-c/bindings/cpp/src/link_options.cpp (line 27)
<https://reviews.apache.org/r/39222/#comment159857>

    In C++11 we should support the use of vector initializers.
    
    In C++03 a chaining op() would avoid the arbitrary "only 4" restriction and multiple constructors:
    
        link_options& link_options::operator()(const link_option &l) { push_back(&l);
return *this }
        
        // User code
        foo.create_sender(..., link_options(opt0)(opt1)(opt2)(opt3)(opt4))


The internals are all good but I think we need to work on the user interface - we need to
have an example of use so we can see exactly how it looks. I think this design means you can't
create option objects as temporaries and pass them in-line, so it looks like:

    opt1 x(value1)
    opt2 y(value2)
    link_options opts(opt1, opt2)
    create_sender(

- Alan Conway


On Oct. 12, 2015, 6:56 a.m., Cliff Jansen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39222/
> -----------------------------------------------------------
> 
> (Updated Oct. 12, 2015, 6:56 a.m.)
> 
> 
> Review request for qpid, Alan Conway, Andrew Stitcher, and Justin Ross.
> 
> 
> Bugs: PROTON-865
>     https://issues.apache.org/jira/browse/PROTON-865
> 
> 
> Repository: qpid-proton-git
> 
> 
> Description
> -------
> 
> Link options, provided much like the Python client.
> 
> Includes examples from Python for selector and browser.
> 
> Additional link and terminus properties were added as required.
> 
> 
> Diffs
> -----
> 
>   examples/cpp/CMakeLists.txt 34edb83 
>   examples/cpp/queue_browser.cpp PRE-CREATION 
>   examples/cpp/selected_recv.cpp PRE-CREATION 
>   proton-c/bindings/cpp/CMakeLists.txt 45e47b2 
>   proton-c/bindings/cpp/include/proton/connection.hpp 08b6ad9 
>   proton-c/bindings/cpp/include/proton/link.hpp 88b5368 
>   proton-c/bindings/cpp/include/proton/link_options.hpp PRE-CREATION 
>   proton-c/bindings/cpp/include/proton/terminus.hpp 4bb9651 
>   proton-c/bindings/cpp/src/connection.cpp 84c79a6 
>   proton-c/bindings/cpp/src/link.cpp 12c8790 
>   proton-c/bindings/cpp/src/link_options.cpp PRE-CREATION 
>   proton-c/bindings/cpp/src/terminus.cpp 2c960d6 
> 
> Diff: https://reviews.apache.org/r/39222/diff/
> 
> 
> Testing
> -------
> 
> Linux.  Checked attach frames compared to Python.
> 
> 
> Thanks,
> 
> Cliff Jansen
> 
>


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