> On March 17, 2014, 8: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. > > Bernd Mathiske wrote: > 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"! > > > Ben Mahler wrote: > Except that we haven't been making the assessment when writing > constructors in most cases (especially in the Mesos specific code), so it's > strictly easier than thinking about conversion semantics all the time. Of > course, not making the assessment could be argued to be a dangerous thing. > You're right that there are potentially some readers of the Mesos code that > spend time wondering why we didn't use 'explicit', that seems a bit > unfortunate. > > I think it's perfectly reasonable to be explicit (no pun intended) about > conversion semantics in stout and libprocess, since these are libraries. > In the Mesos code, I'm still not sold on the value gained, but I'd rather > keep the same style across stout/libprocess/Mesos. > > So, I'm ok with adding this style requirement to keep things consistent, > can you make sure to follow up in the style guide and help remind us to > enforce this style during reviews or through a lint tool? > > Thanks Bernd!
Sorry I'm getting to this thread late. We all know the right solution here is to amend the C++ language to add the keyword 'implicit' and require all single argument constructors to either be decorated with 'implicit' or 'explicit'. ;) Who's going to write the proposal? The good news is that once that happens (C++42 maybe?) we'll be well prepared to do a s|/*implicit*/|implicit| in one fell swoop. Now here's hoping (pleading) for good attention to detail during reviews (in the absence of a tool) so that we don't have to do this exercise again. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19086/#review37449 ----------------------------------------------------------- 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 > >
