> On Feb. 25, 2016, 10:09 p.m., Andrew Stitcher wrote: > > proton-c/CMakeLists.txt, line 251 > > <https://reviews.apache.org/r/43742/diff/2/?file=1264517#file1264517line251> > > > > Use list (APPEND ...) - it's a little bit clearer. And used elsewhere > > in this CMakeLists.txt I'm pretty sure.
Tried it, doesn't work. To my surprise (and apparently yours) cmake does not treat this variable as a list but as a literal string to paste into the command line. If you use list(append...) the command line has the cmake ';' separator in it. I hate cmake. > On Feb. 25, 2016, 10:09 p.m., Andrew Stitcher wrote: > > proton-c/bindings/cpp/include/proton/byte_array.hpp, line 44 > > <https://reviews.apache.org/r/43742/diff/2/?file=1264519#file1264519line44> > > > > Didn't we agree to separate the friend declarations and function > > declaration/definitions? We did but we need to do it everywhere, I didn't include that as part of this commit as most of the work was done before we agreed that. > On Feb. 25, 2016, 10:09 p.m., Andrew Stitcher wrote: > > proton-c/bindings/cpp/include/proton/byte_array.hpp, line 40 > > <https://reviews.apache.org/r/43742/diff/2/?file=1264519#file1264519line40> > > > > Missing C++11 cbegin, cend? > > > > Don't you need to export some types too for correct iterator support. I started to do a full std::array compatible with all the typedefs etc. and it got very complicated so I trimmed it down to just what we need. If we do the full treatment there are other things missing: rbegin(), rend(), at() throws std::range_error etc. The other option is to keep this simple declaration for C++03 and in C++11 do template <std::size_t N> typedef byte_array std::array<char, N> I'd lean towards the latter, what's your preference? > On Feb. 25, 2016, 10:09 p.m., Andrew Stitcher wrote: > > proton-c/bindings/cpp/include/proton/endpoint.hpp, line 81 > > <https://reviews.apache.org/r/43742/diff/2/?file=1264525#file1264525line81> > > > > Why are endpoints comparable? I can only see that they need to be > > equality compared. What does ordering mean in the context of endpoints? Agreed, already fixed. > On Feb. 25, 2016, 10:09 p.m., Andrew Stitcher wrote: > > proton-c/bindings/cpp/include/proton/object.hpp, line 38 > > <https://reviews.apache.org/r/43742/diff/2/?file=1264528#file1264528line38> > > > > Actually why do we need more than equality comparison on objects? > > > > They can be put into unordered containers fine without any ordering. They can't be used as std::map keys without ordering. Users will frequently need to associate app resources with proton objects, a map keyed by the object is a natural way to do it. Since the object wraper is really just a pointer, and pointers can be used as map keys, it seems like objects should be usable that way too. Providing a C++ API for proton contexts/records will solve this problem for *some* cases but not all. Retrieving contexts is not thread-safe, and even if it was (I *strongly* advise against that), it's would still be a reasonable design choice to use separate maps for different parts of the application instead of per-object locks that will be contended by every part of the application. I was forced to think about this a lot in the go binding, happy to elaborate. > On Feb. 25, 2016, 10:09 p.m., Andrew Stitcher wrote: > > proton-c/bindings/cpp/include/proton/timestamp.hpp, line 24 > > <https://reviews.apache.org/r/43742/diff/2/?file=1264531#file1264531line24> > > > > You were going to add a now() member. Which only needs to call the > > underlying proton-c pn_i_now(). Yes. I will expose pn_timestamp_now() in the C library which is how it should have been in the first place. Nice job with the symbol hiding, first time I caught a missing PN_EXTERN on linux :) > On Feb. 25, 2016, 10:09 p.m., Andrew Stitcher wrote: > > proton-c/bindings/cpp/include/proton/types.hpp, line 31 > > <https://reviews.apache.org/r/43742/diff/2/?file=1264533#file1264533line31> > > > > What does the XXX comment here mean? It doesn't seem relevant to this > > file, can we just remove it? > > Justin Ross wrote: > I added that comment. It's a minor consistency question. In every > other instance we use the style 'include "protonheader.h"'. Is this > different for a reason? > > Andrew Stitcher wrote: > In that case I don't understand the question. > > The two different styles of #include actually set up different include > search paths in theory (although in practice in many Linux environments the > two paths are the same) > > C/C++ Style background: #include <> indicates using a system/external > header which is correct for this use. #include "" indicates using only non > system headers which would be correct for use to include some project > internal header. > > So in the exported header files every #include should be <> with the full > path. In the internal only source and header files you should use <> to get > the exported headers (IMO as this is a little debatable) and use "" for > internal header files. +1 what he said. As to the debatable part - Andrew and I agree so it's not debatable. We Are Right. > On Feb. 25, 2016, 10:09 p.m., Andrew Stitcher wrote: > > proton-c/bindings/cpp/src/duration.cpp, line 29 > > <https://reviews.apache.org/r/43742/diff/2/?file=1264543#file1264543line29> > > > > nit: shouldn't you be using a type trait here instead of int64_t? Or > > doesn't duration have such a trait? I added typedefs timestamp::numeric_type and duration::numeric_type, so now it is `std::numeric_traits<duration::numeric_type>::max()`. That way you can use all the std:: traits stuff, such as is_signed etc. > On Feb. 25, 2016, 10:09 p.m., Andrew Stitcher wrote: > > proton-c/bindings/cpp/src/uuid.cpp, line 74 > > <https://reviews.apache.org/r/43742/diff/2/?file=1264555#file1264555line74> > > > > This is a really crappy random number generator and gives warnings from > > coverity, can we use something better? At least in the C++11 case? Sure. What? > On Feb. 25, 2016, 10:09 p.m., Andrew Stitcher wrote: > > proton-c/bindings/cpp/src/message.cpp, line 94 > > <https://reviews.apache.org/r/43742/diff/2/?file=1264546#file1264546line94> > > > > What happened to the type of .as_uuid so that you need a > > reinterpret_cast? This change looks alarming! Why not just > > ```message_id(v.u.as_uuid)```? I thought the explicit constructors were > > going away. Even with a copy constructor in the way the compiler can elide > > it - certainly it can in C++11 if there is a suitable message_id move > > constructor. Yes that was nasty, already fixed - message_id has a private pn_atom_t constructor and is a friend of message. - Alan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43742/#review120777 ----------------------------------------------------------- On Feb. 25, 2016, 9:31 p.m., Alan Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43742/ > ----------------------------------------------------------- > > (Updated Feb. 25, 2016, 9:31 p.m.) > > > Review request for qpid, Andrew Stitcher, Cliff Jansen, and Justin Ross. > > > Bugs: PROTON-1138 > https://issues.apache.org/jira/browse/PROTON-1138 > > > Repository: qpid-proton-git > > > Description > ------- > > PROTON-1138: Add proton::uuid type. > > Official C++ representation of a UUID, removed amqp_uuid. > > Also added proton::byte_array base class for this and other fixed-size proton > types that lack C++ native equivalents so need to be presented in C++ as > byte-arrays. > > PROTON-1138: Add proton::timestamp type. > > - Removed amqp_timestamp. > - Got rid of public duration::milliseconds data member. > - Made duration and timestamp consistent with const int64_t ms() accessor, > explicit constructor and op=(int64_t) > - Completed duration/timestamp arithmetic. > > NOTE changed duration to use signed int64_t. Durations can be added and > subtracted and negative durations can occur during calculations. Also AMQP > timestamp is defined as signed so we should be consistent. > > UNIX time_t is traditionally unsigned because signed 32 bits is too small to > express a useful time range after the epoch (frankly unsigned is too, watch > out > in 2038 :) 64 bit timestamps and durations do not require this restriction. > > PROTON-1138: Add proton::decimal types. > > > PROTON-1138: Simplify comparable class, make comparable inheritance private. > > Simplified the comparable class. > > - comparable<T> is a private empty base class that exists only to instantiate > operator friends of T. > - comparable<T> is never used in any other way. > - for any compiler with even rudimentary empty-base-class support, this has 0 > run-time overhead. > > I did some research to try and eliminate inheritance entirely. There are 2 > well > known approaches. > > 1. boost::operators uses exactly the approach above. > 2. std::rel_ops uses unconstrained templates that match anything with op == > or < operator, we already agreed that approach is unsuitable for public API. > > I tried type-trait approaches but could not find one that was better than the > above. IMO inheritance is the right fit: It has no run-time overhead, it is > easy to understand, and we *want* "is-a" inheritance semantics: if `object` is > comparable then sub-classes of `object` should be comparable. (That is the > part > that is hard to achieve with type-traits. When I realized I was trying to > reinvent inheritance I decided to let go.) > > PROTON-1138: c++: Extend_CXX_FLAGS in CMakeLists.txt, don't overwrite user > setting. > > > Diffs > ----- > > proton-c/CMakeLists.txt 862a520f5103cdd5a08539fd3bfc9b49eeabdef5 > proton-c/bindings/cpp/CMakeLists.txt > 7c1c41dfa1f235864c23a459209ed024b81fa7cd > proton-c/bindings/cpp/include/proton/byte_array.hpp PRE-CREATION > proton-c/bindings/cpp/include/proton/comparable.hpp > 6c6424963929ae90765e84ede0bbb32fd15feb85 > proton-c/bindings/cpp/include/proton/decimal.hpp PRE-CREATION > proton-c/bindings/cpp/include/proton/decoder.hpp > 3dab9c94f4875a9a8ef4323f30ee999fd01f8427 > proton-c/bindings/cpp/include/proton/duration.hpp > f2068494b213ac67034fab59364da1ea790af505 > proton-c/bindings/cpp/include/proton/encoder.hpp > 187641237d0b31f712f506a61b4c1db7214e0c17 > proton-c/bindings/cpp/include/proton/endpoint.hpp > 8a8712ed22c3dacdae4f21a65268a313a19db66b > proton-c/bindings/cpp/include/proton/message.hpp > 045143f5b22e0275d193f058bdfe850db472bac2 > proton-c/bindings/cpp/include/proton/message_id.hpp > 0adef07e45ccc03775decb8a4fb82c8f8fbe0cf3 > proton-c/bindings/cpp/include/proton/object.hpp > 4e12e6fe9c4800c5afe6e6b5fd01dc10f7a213f1 > proton-c/bindings/cpp/include/proton/reactor.hpp > 82514058ccb33820b6aa5635b303e3f5b32d6fbd > proton-c/bindings/cpp/include/proton/scalar.hpp > 3975301e8b1380d92d5a312c475cca6069ff86a2 > proton-c/bindings/cpp/include/proton/timestamp.hpp PRE-CREATION > proton-c/bindings/cpp/include/proton/type_traits.hpp > 56f503ce37ef54cf6636dd4632e8846b4c0ae6e1 > proton-c/bindings/cpp/include/proton/types.hpp > ee93082965e896f736d60d894767b4109ecdee87 > proton-c/bindings/cpp/include/proton/url.hpp > ccf3f1ed389d080a1790069ce7a2c9e1029dfef1 > proton-c/bindings/cpp/include/proton/uuid.hpp PRE-CREATION > proton-c/bindings/cpp/include/proton/value.hpp > b2c1c0bb01350e0228c711a3d43fa1310d6e2c31 > proton-c/bindings/cpp/src/condition.cpp > 9220fd7aa672fb3a57482a0e30af65ee3fb7c691 > proton-c/bindings/cpp/src/connection_engine.cpp > 89d901e588d5dffb1e760509d475bf56c6df9bf3 > proton-c/bindings/cpp/src/connection_options.cpp > 2751aaa4f663dedf24d24b25b39da47fc7c50471 > proton-c/bindings/cpp/src/container_impl.cpp > 8527b68feb2294de966020762874708d5e3f9a6b > proton-c/bindings/cpp/src/data.cpp 6783dae23b93114110f8f6ef90b3fe993ea9a334 > proton-c/bindings/cpp/src/decoder.cpp > f775001c40f6da158ad2ef11022a4e24038d13ca > proton-c/bindings/cpp/src/duration.cpp > 8c8263e411f1f9b064bbee4d4924d4fb728b9354 > proton-c/bindings/cpp/src/encoder.cpp > 58b41b3745ef2224ca0bca40c1481d88fe0e2a8f > proton-c/bindings/cpp/src/engine_test.cpp > cb8e38908396f90f23441b7070a222e3fc907f27 > proton-c/bindings/cpp/src/message.cpp > 2358da23bec8a354aa5db63397acf59236488456 > proton-c/bindings/cpp/src/message_test.cpp > 988aa34891839088ad7f256c5263266339be8ec8 > proton-c/bindings/cpp/src/reactor.cpp > 59db5e082e06727128c283361e1967c6dbc38573 > proton-c/bindings/cpp/src/reconnect_timer.cpp > 2a44063bc41c98108a26155e5b4577d05b032b87 > proton-c/bindings/cpp/src/scalar.cpp > 3379c015c07e337cc35ef1ebfc32554b658d4607 > proton-c/bindings/cpp/src/scalar_test.cpp > a9700b0133830fa8b9e01fb1f761805a218bd8b6 > proton-c/bindings/cpp/src/types.cpp > 1ace1a17fdf46b8ad068c395267fe24dedc59913 > proton-c/bindings/cpp/src/types_internal.hpp PRE-CREATION > proton-c/bindings/cpp/src/uuid.hpp e8ee908e95e8eff0f1388fe8fe41218dc2d504c9 > proton-c/bindings/cpp/src/uuid.cpp 6e8367fc52de5cd776cd72faeb843c90279d61e2 > tests/tools/apps/cpp/reactor_send.cpp > 76fa9dcb4636315ee1ea760732946b9ac403c4b9 > > Diff: https://reviews.apache.org/r/43742/diff/ > > > Testing > ------- > > ctest -R cpp # not tested across c++ versions or platforms yet. > > > Thanks, > > Alan Conway > >
