> On Oct. 30, 2015, 1:19 a.m., Alan Conway wrote:
> > I have what I think is a simpler implementation of connection options, I
> > can't update the reviewboard look at:
> > https://github.com/alanconway/qpid-proton/commit/7eb1e87c1fdc8a72422b36770acbe0d3fa2deb1c
> >
> > The logic is Cliff's (if I didn't miss anything) but the impl is shorter
> > and I think more readable. If we like this we could do the same for link
> > options.
> >
> > NO-JIRA: Simpler implementation of connection options.
> >
> > This is a simpler implementation of connection_options
> >
> > - More compact, option tests and apply logic together in one apply()
> > function.
> > - No limit on number of options that can be chained in call to connect()
> > - Simple value semantics. No need for virtual clone(), inheritance,
> > pointers.
> > - Fewer heap allocations - one per option set, not per individual option.
> >
> > Options class is pimpl with no virtuals so no binary compat issues adding
> > new options.
> >
> > /** Options for creating a connection.
> > *
> > * Options can be "chained" like this:
> > *
> > * c = container.connect(url,
> > connection_options().handler(h).max_frame_size(1234));
> > *
> > * You can also create an options object with common settings and use it as
> > a base
> > * for different connections that have mostly the same settings:
> > *
> > * connection_options opts;
> > * opts.idle_timeout(1000).max_frame_size(10000);
> > * c1 = container.connect(url1, opts.handler(h1));
> > * c2 = container.connect(url2, opts.handler(h2));
> > *
> > * Normal value semantics, copy or assign creates a separate copy of the
> > options.
> > */
>
> Andrew Stitcher wrote:
> I think I prefer Alan's implementation.
>
> I note that using variadic templates (in C++11 and on) you can get a
> perhaps maore natural connection constructor allowing multiple options -
> Either as a constructor for connection_options or at the end of the
> connection constructor. So we should definately allow that in a modern C++
> compiler, but we can add that later.
>
> That would look like:
>
> Either
> c = container.connect(url, handler(h), max_frame_size(1234));
> Or
> c = container.connect(url,
> connection_options(handler(h).max_frame_size(1234)));
>
> Andrew Stitcher wrote:
> Oops - for that last I meant:
> c = container.connect(url, connection_options(handler(h),
> max_frame_size(1234)));
I'm assuming your Oops was meant to say:
c = container.connect(url,
connection_options().handler(h).max_frame_size(1234));
Also one issue with the varadic approach: presumably your option names would
have to be in the global scope, so "handler" wouldn't work - it works in my
approach because it is a member of connection_options. One way to deal with
that would be to put the varadic versions in a namespace:
namespace copts {
// define handler, max_frame_size etc.
}
// Now you have a choice:
c = container.connect(url, copts::handler(h), copts::max_frame_size(1234))
// OR
using namespace copts;
c = container.connect(url, handler(h), max_frame_size(1234))
That's how we managed the naming of boost optional arguments waaay back in the
old days of the qpid::client.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39694/#review104505
-----------------------------------------------------------
On Oct. 27, 2015, 6:21 p.m., Cliff Jansen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39694/
> -----------------------------------------------------------
>
> (Updated Oct. 27, 2015, 6:21 p.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
> -------
>
> This provides connection options (broadly similar to link options).
>
> As suggested by Alan, the collection connection_options is a subclass of the
> base connection_option so that they can be grouped and nested.
>
> They can also be copied and persisted via respective clone methods.
> Reconnect and transport options need to live as long as the connection. The
> handler option is special and needs to be extracted before other options to
> be available when the connection is created.
>
> Incoming (listener) connection options are perhaps missing a feature. You
> can set global server connection options on a container, and you can set
> separate ssl_domain creds per listener, but you cannot set other connection
> options (idle_timeout/max_frame_size etc) per listener. This is reflected in
> Proton-C's acceptor and the readable events that it intercepts. There is no
> way to query the C API to find out which listener is the parent of an
> incoming connection or transfer other listener-specific context to the
> connection.
>
> As suggested in the link options review, I attempted to provide an
> initializer_list<foo> constructor and append, but that requires array
> elements of identical size. The varying size of the connection option types
> is unhelpful here. Maybe they should be counted_ptr based pimpl structs of
> identical size.
>
> This patch can also be viewed as the "reconnect" branch of
> https://github.com/cliffjansen/qpid-proton.
>
>
> Diffs
> -----
>
> examples/cpp/CMakeLists.txt 34edb83
> examples/cpp/connection_options.cpp PRE-CREATION
> proton-c/bindings/cpp/CMakeLists.txt 9f423fb
> proton-c/bindings/cpp/include/proton/connection_options.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/container.hpp 059416f
> proton-c/bindings/cpp/include/proton/reconnect_timer.hpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/transport.hpp 3d602b7
> proton-c/bindings/cpp/src/blocking_connection_impl.cpp 8abd24b
> proton-c/bindings/cpp/src/connection.cpp 1b35e03
> proton-c/bindings/cpp/src/connection_options.cpp PRE-CREATION
> proton-c/bindings/cpp/src/connector.hpp b311cc9
> proton-c/bindings/cpp/src/connector.cpp 49660ea
> proton-c/bindings/cpp/src/container.cpp 13f12d9
> proton-c/bindings/cpp/src/container_impl.hpp ec356e2
> proton-c/bindings/cpp/src/container_impl.cpp fc97a3e
> proton-c/bindings/cpp/src/reconnect_timer.cpp PRE-CREATION
> proton-c/bindings/cpp/src/transport.cpp 58114ae
>
> Diff: https://reviews.apache.org/r/39694/diff/
>
>
> Testing
> -------
>
> Linux only
>
>
> Thanks,
>
> Cliff Jansen
>
>
