> 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))); > > Alan Conway wrote: > 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.
No the correction was indeed correct - it uses a variadic constructor for connection_options instead of for connection - your correction is exactly what was already there! Your point about names is a good one, I think namespacing works. Actually going in this direction does take you quite far towards named parameters (if you want to take that journey!) - Andrew ----------------------------------------------------------- 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 > >
