----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19086/#review37449 -----------------------------------------------------------
I like the addition of the 'explicit' keyword on those single argument constructors that are not intended to be used for implicit conversion. However, I'd like to understand the problem we're trying to solve here. What motivated this change? Did we start with a problem, and then solve it through this style? Or did we find that we were not conforming to Google's C++ style guide, and decide it's a good idea to add this? If it's the latter, I would like to see a more persuasive conversation around introducing a style change here. As I understand it, we're looking to prevent accidental implicit conversion from occurring in our code silently, and resulting in some bug at run-time. In practice, I've not found this to be a problem working on Mesos / stout / libprocess. I do recall one issue where an implicit UPID() conversion was occurring and resulting in an unexpected logging statement (but I'd love to add 'explicit' on UPID!). I've found that the style of C++ we use in Mesos has not been conducive to causing implicit conversion issues. So, I agree with adding 'explicit' keywords where we think accidents are likely. :) But, I'm a bit hesitant to start *requiring* ourselves to use 'explicit' or '/*implicit*/' *everywhere* in the code where we have single argument constructors (including small 'struct' objects). For the most part, I've found context to be a sufficient indicator of whether 'implicit' semantics are truly desired. A suggestion for us to consider: 1. We insert 'explicit' as we've done here, where we think accidents are likely (e.g. UPID). 2. We do not use '/*implicit*/'. 3. We do not require either '/*implicit*/' or 'explicit' as part of our style guide, I'm concerned about the readability overhead. While the current path of this change is indeed the most *defensive* approach, I'm not convinced that introducing a style *requirement* for this would be (1) solving a real problem, and (2) providing significant value. Would love to hear from others here! - Ben Mahler On March 13, 2014, 6:53 p.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19086/ > ----------------------------------------------------------- > > (Updated March 13, 2014, 6:53 p.m.) > > > Review request for mesos, Benjamin Hindman and Jie Yu. > > > Bugs: MESOS-1057 > https://issues.apache.org/jira/browse/MESOS-1057 > > > Repository: mesos-git > > > Description > ------- > > Applied either "explicit" or "/*implicit*/" to all single-arg constructors in > lib process that do not take the constructed type as parameter. > > Sorted out which denotation to choose by compiling mess and fixing all the > compile errors and by estimating whether implicit conversions might be > intended by the original authors. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp d43f1b5 > 3rdparty/libprocess/3rdparty/stout/include/stout/bytes.hpp 72dea4d > 3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp ea5017f > 3rdparty/libprocess/3rdparty/stout/include/stout/error.hpp 12ba1ca > 3rdparty/libprocess/3rdparty/stout/include/stout/exit.hpp aaccbb4 > 3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp d6b070f > 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 0ce002c > 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 3305d13 > 3rdparty/libprocess/3rdparty/stout/include/stout/os/fork.hpp 88b5797 > 3rdparty/libprocess/3rdparty/stout/include/stout/os/signals.hpp f32130a > 3rdparty/libprocess/3rdparty/stout/include/stout/proc.hpp 201e4b8 > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 6fe795f > 3rdparty/libprocess/3rdparty/stout/include/stout/result.hpp 3a10379 > 3rdparty/libprocess/3rdparty/stout/include/stout/set.hpp ea8a13f > 3rdparty/libprocess/3rdparty/stout/include/stout/some.hpp 1a71ac4 > 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp d99b75a > 3rdparty/libprocess/include/process/c++11/deferred.hpp 57a3df5 > 3rdparty/libprocess/include/process/deferred.hpp c18619e > 3rdparty/libprocess/include/process/event.hpp ca407ec > 3rdparty/libprocess/include/process/future.hpp 27b0970 > 3rdparty/libprocess/include/process/http.hpp 7f549ba > 3rdparty/libprocess/include/process/owned.hpp 4a03ea4 > 3rdparty/libprocess/include/process/pid.hpp 5a77dbc > 3rdparty/libprocess/include/process/process.hpp 37283ea > 3rdparty/libprocess/include/process/shared.hpp 8f5b59b > 3rdparty/libprocess/include/process/time.hpp bd31211 > 3rdparty/libprocess/include/process/timeout.hpp 4634b9f > 3rdparty/libprocess/include/process/tuples/details.hpp 34a9fb5 > 3rdparty/libprocess/include/process/tuples/tuples.hpp 672ba6c > 3rdparty/libprocess/src/decoder.hpp a34c005 > 3rdparty/libprocess/src/encoder.hpp 4810e99 > 3rdparty/libprocess/src/net.hpp 2fdc62a > 3rdparty/libprocess/src/synchronized.hpp 7e0efe2 > > Diff: https://reviews.apache.org/r/19086/diff/ > > > Testing > ------- > > No compile errors occur now and Mesos still seems to work. Compiled and ran > simple test frameworks on MacOSX 10.9 with Clang 3.3 and on Ubuntu 13.10 with > gcc4.7.1. Note: the former exercised the C++11 subdir in lib > process/include/process/c++11, whereas the latter did not. So we have both > variants of the affected file "deferred.hpp" covered. > > > Thanks, > > Bernd Mathiske > >
