----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43742/#review120777 -----------------------------------------------------------
proton-c/CMakeLists.txt (line 251) <https://reviews.apache.org/r/43742/#comment182259> Use list (APPEND ...) - it's a little bit clearer. And used elsewhere in this CMakeLists.txt I'm pretty sure. proton-c/bindings/cpp/include/proton/byte_array.hpp (line 40) <https://reviews.apache.org/r/43742/#comment182261> Missing C++11 cbegin, cend? Don't you need to export some types too for correct iterator support. proton-c/bindings/cpp/include/proton/byte_array.hpp (line 44) <https://reviews.apache.org/r/43742/#comment182264> Didn't we agree to separate the friend declarations and function declaration/definitions? proton-c/bindings/cpp/include/proton/comparable.hpp (line 32) <https://reviews.apache.org/r/43742/#comment182263> I think we agreed to declare and define friends separately (for clarities sake) proton-c/bindings/cpp/include/proton/endpoint.hpp (line 81) <https://reviews.apache.org/r/43742/#comment182265> Why are endpoints comparable? I can only see that they need to be equality compared. What does ordering mean in the context of endpoints? proton-c/bindings/cpp/include/proton/object.hpp (line 38) <https://reviews.apache.org/r/43742/#comment182266> Actually why do we need more than equality comparison on objects? They can be put into unordered containers fine without any ordering. proton-c/bindings/cpp/include/proton/timestamp.hpp (line 24) <https://reviews.apache.org/r/43742/#comment182267> You were going to add a now() member. Which only needs to call the underlying proton-c pn_i_now(). proton-c/bindings/cpp/include/proton/types.hpp (line 30) <https://reviews.apache.org/r/43742/#comment182268> What does the XXX comment here mean? It doesn't seem relevant to this file, can we just remove it? proton-c/bindings/cpp/src/duration.cpp (line 29) <https://reviews.apache.org/r/43742/#comment182269> nit: shouldn't you be using a type trait here instead of int64_t? Or doesn't duration have such a trait? proton-c/bindings/cpp/src/message.cpp (line 94) <https://reviews.apache.org/r/43742/#comment182270> 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. proton-c/bindings/cpp/src/uuid.cpp (line 74) <https://reviews.apache.org/r/43742/#comment182273> 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? tests/tools/apps/cpp/reactor_send.cpp (line 77) <https://reviews.apache.org/r/43742/#comment182275> should be timstamp::now() - when you've added it! - Andrew Stitcher 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 > >
