> On March 17, 2014, 1:05 p.m., Ben Mahler wrote: > > 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! > > Bernd Mathiske wrote: > It was my impression that stricter adherence to the Google style was > wanted here. However, for me this implies that you want to leave breadcrumbs > indicating intent when you do not use "explicit". Otherwise you end up > revisiting solved but ambiguous-looking cases. > > If you do not want to make it a requirement to put either "explicit" or > "/*implicit*/", that's fine, too, but maybe then this whole JIRA issue should > have been rejected in the first place? If we leave it at just adding a few > "explicit" insertions now, then another ticket that asks for more of them is > bound to appear in the future. > > Or is there a simple rule (of thumb) that makes clear what is intended > when there is neither explicit nor /*implicit*/ present? I doubt that. > > Absent such a rule I see two favorable outcomes, either of which I would > support: > - make nothing mandatory, drop this issue and never ever post such an > issue again > - make mandatory that either annotation is present every time > > As a defensive programmer, never shy of writing more characters, I would > lean towards the latter. I do not see any readability overhead in that, given > my own definition of that notion. On the contrary: if there is no > /*implicit*/ you need to investigate further to evaluate what is the matter > with that constructor, i.e. read more! That is the opposite of readability in > my book. Code should be as LOCALLY readable as possible, i.e. without having > to browse in other places to understand exactly what is intended. > > I agree that solving this issue probably has too little value to bother, > though. That's why I would also support dropping it. Just please don't go > half way! > > Ben Mahler wrote: > Thank you Bernd for the very thoughtful response :) > > I think both of your alternatives sound great to me. > > My "rule of thumb" would be: If it is a library "wrapper" type (often > implies a template), then implicit is desired. Otherwise, it is not desired. > Future / Try / Result / Option / Resources (convenience wrapper around > protobuf) / Attributes (convenience wrapper around protobuf) / Log::Position > / etc fit the rule to me and __Exit / Network / > ZooKeeperMasterContenderProcess do not fit this rule. I can see how there is > wiggle room here, like SystemStatus. > > Despite being the most defensive approach, my inclination is to not > bother with this, since we would be forced to always think about conversion > semantics when it's not clear that we're gaining a lot of value.
So, you are leaning to make it easier for the code writer at the expense of the code reader? Understandable. But this only works if the code in question is really rarely read. Every time you write a new constructor, you have to assess how "important" your piece of code at hand will be in the unforeseeable future. Is that easier than thinking about the conversion semantics? I doubt that. So I propose this setup: 1. ALWAYS put "explicit". NEVER leave it blank. 2. Only if you are convinced that implicit is wanted, then put /*implicit*/. So, if you don't want to think, just write "explicit"! - Bernd ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19086/#review37449 ----------------------------------------------------------- On March 13, 2014, 11:53 a.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19086/ > ----------------------------------------------------------- > > (Updated March 13, 2014, 11:53 a.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 > >
