Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1413763003:
Introduce rtc::Maybe<T>, which either contains a T or not. (Closed)
|
Created:
5 years, 2 months ago by kwiberg-webrtc Modified:
5 years, 2 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionIntroduce rtc::Maybe<T>, which either contains a T or not.
It's a simple std::experimental::optional-wannabe. For simplicity and
portability, it still secretly contains a (default-constructed) T when
it's supposedly empty. This restriction is fine for simple types.
One important application is for the return type of functions. For
example, a function which either returns a size_t or fails can return
rtc::Maybe<size_t>.
BUG=webrtc:5028
R=andrew@webrtc.org, mgraczyk@chromium.org
Committed: https://crrev.com/6e587200db7ddae0d33e5187b73acdd41175584c
Cr-Commit-Position: refs/heads/master@{#10353}
Patch Set 1 #Patch Set 2 : MSVC lacks support for C++11 =default for move construction and assignment #
Total comments: 2
Patch Set 3 : Make constructors explicit #
Total comments: 31
Patch Set 4 : Bug fix moved-from documentation #Patch Set 5 : Add value_or method #Patch Set 6 : Try to make clang/gcc and MSVC do things in the same order #Patch Set 7 : Try again #Patch Set 8 : Make constructors implicit again #
Messages
Total messages: 33 (8 generated)
Description was changed from ========== Introduce rtc::Maybe<T>, which either contains a T or not. It's a simple std::experimental::optional-wannabe. For simplicity and portability, it still secretly contains a (default-constructed) T when it's supposedly empty. This restriction is fine for simple types. One important application is for the return type of functions. For example, a function which either returns a size_t or fails can return rtc::Maybe<size_t>. BUG=webrtc:5028 ========== to ========== Introduce rtc::Maybe<T>, which either contains a T or not. It's a simple std::experimental::optional-wannabe. For simplicity and portability, it still secretly contains a (default-constructed) T when it's supposedly empty. This restriction is fine for simple types. One important application is for the return type of functions. For example, a function which either returns a size_t or fails can return rtc::Maybe<size_t>. BUG=webrtc:5028 ==========
kwiberg@webrtc.org changed reviewers: + andrew@webrtc.org, henrik.lundin@webrtc.org, solenberg@webrtc.org
Please have a look. I wanted to use this for returning values from functions in another CL. Note that this CL triggers presubmit warnings for three reasons: 1. It implements move construction and move assignment, which is generally allowed by the Google C++ style guide but still requires Special Permission according to the Chromium C++11 cheat sheet. I'm arguing that Maybe<T>, like scoped_ptr<T>, is precisely the sort of class that's intended to be exempt from this. 2. It has non-explicit one-argument constructors for allowing implicit conversion from T to Maybe<T>. This is forbidden by the Google C++ style guide, except for classes "that are intended to be transparent wrappers around other classes". My position is that Maybe<T> is such a transparent wrapper. 3. As a consequence of the conjunction of (1) and (2), it has a constructor and an operator= that accept a T&&. This triggers a separate alert. By dropping the convenient and harmless implicit conversion from T to Maybe<T>, we get rid of (2) and (3); I can do that if desired. Getting rid of (1) means dropping move support, which seems bad in a generic type like this.
On 2015/10/19 11:55:32, kwiberg-webrtc wrote: > Please have a look. I wanted to use this for returning values from functions in > another CL. > > Note that this CL triggers presubmit warnings for three reasons: > > 1. It implements move construction and move assignment, which is generally > allowed by the Google C++ style guide but still requires Special Permission > according to the Chromium C++11 cheat sheet. I'm arguing that Maybe<T>, like > scoped_ptr<T>, is precisely the sort of class that's intended to be exempt from > this. > > 2. It has non-explicit one-argument constructors for allowing implicit > conversion from T to Maybe<T>. This is forbidden by the Google C++ style guide, > except for classes "that are intended to be transparent wrappers around other > classes". My position is that Maybe<T> is such a transparent wrapper. > > 3. As a consequence of the conjunction of (1) and (2), it has a constructor > and an operator= that accept a T&&. This triggers a separate alert. > > By dropping the convenient and harmless implicit conversion from T to Maybe<T>, > we get rid of (2) and (3); I can do that if desired. Getting rid of (1) means > dropping move support, which seems bad in a generic type like this. Explicitly creating return values should be ok so I'd get rid of 2,3. Additionally, there is a related class here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... Just with a little more clumsy interface. Except for ToString, GetWithDefaultIfUnset and operator==/!= you should be able to replace Settable<> with Maybe<>. Or vice versa.
https://codereview.webrtc.org/1413763003/diff/20001/webrtc/base/maybe.h File webrtc/base/maybe.h (right): https://codereview.webrtc.org/1413763003/diff/20001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:57: Maybe& operator=(const T& val) { if (this != &val) { ...
On 2015/10/19 12:43:29, the sun wrote: > On 2015/10/19 11:55:32, kwiberg-webrtc wrote: > > Please have a look. I wanted to use this for returning values from functions > in > > another CL. > > > > Note that this CL triggers presubmit warnings for three reasons: > > > > 1. It implements move construction and move assignment, which is generally > > allowed by the Google C++ style guide but still requires Special Permission > > according to the Chromium C++11 cheat sheet. I'm arguing that Maybe<T>, like > > scoped_ptr<T>, is precisely the sort of class that's intended to be exempt > from > > this. > > > > 2. It has non-explicit one-argument constructors for allowing implicit > > conversion from T to Maybe<T>. This is forbidden by the Google C++ style > guide, > > except for classes "that are intended to be transparent wrappers around other > > classes". My position is that Maybe<T> is such a transparent wrapper. > > > > 3. As a consequence of the conjunction of (1) and (2), it has a constructor > > and an operator= that accept a T&&. This triggers a separate alert. > > > > By dropping the convenient and harmless implicit conversion from T to > Maybe<T>, > > we get rid of (2) and (3); I can do that if desired. Getting rid of (1) means > > dropping move support, which seems bad in a generic type like this. > > Explicitly creating return values should be ok so I'd get rid of 2,3. OK. > Additionally, there is a related class here: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > Just with a little more clumsy interface. Except for ToString, > GetWithDefaultIfUnset and operator==/!= you should be able to replace Settable<> > with Maybe<>. Or vice versa. With this sort of class, it's all about the interface, since the implementation is dead simple. So I'd feel uncomfortable using Settable because its deliberately specialized, if only in name. Using Maybe in places that currently use Settable would make sense, though.
https://codereview.webrtc.org/1413763003/diff/20001/webrtc/base/maybe.h File webrtc/base/maybe.h (right): https://codereview.webrtc.org/1413763003/diff/20001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:57: Maybe& operator=(const T& val) { On 2015/10/19 12:43:41, the sun wrote: > if (this != &val) { ... Not possible. this is a Maybe<T>*, &val is a T*.
On 2015/10/19 13:00:07, kwiberg-webrtc wrote: > https://codereview.webrtc.org/1413763003/diff/20001/webrtc/base/maybe.h > File webrtc/base/maybe.h (right): > > https://codereview.webrtc.org/1413763003/diff/20001/webrtc/base/maybe.h#newco... > webrtc/base/maybe.h:57: Maybe& operator=(const T& val) { > On 2015/10/19 12:43:41, the sun wrote: > > if (this != &val) { ... > > Not possible. this is a Maybe<T>*, &val is a T*. Ah, parse error. Thanks.
On 2015/10/19 12:57:57, kwiberg-webrtc wrote: > On 2015/10/19 12:43:29, the sun wrote: > > On 2015/10/19 11:55:32, kwiberg-webrtc wrote: > > > Please have a look. I wanted to use this for returning values from functions > > in > > > another CL. > > > > > > Note that this CL triggers presubmit warnings for three reasons: > > > > > > 1. It implements move construction and move assignment, which is generally > > > allowed by the Google C++ style guide but still requires Special Permission > > > according to the Chromium C++11 cheat sheet. I'm arguing that Maybe<T>, like > > > scoped_ptr<T>, is precisely the sort of class that's intended to be exempt > > from > > > this. > > > > > > 2. It has non-explicit one-argument constructors for allowing implicit > > > conversion from T to Maybe<T>. This is forbidden by the Google C++ style > > guide, > > > except for classes "that are intended to be transparent wrappers around > other > > > classes". My position is that Maybe<T> is such a transparent wrapper. > > > > > > 3. As a consequence of the conjunction of (1) and (2), it has a > constructor > > > and an operator= that accept a T&&. This triggers a separate alert. > > > > > > By dropping the convenient and harmless implicit conversion from T to > > Maybe<T>, > > > we get rid of (2) and (3); I can do that if desired. Getting rid of (1) > means > > > dropping move support, which seems bad in a generic type like this. > > > > Explicitly creating return values should be ok so I'd get rid of 2,3. > > OK. > > > Additionally, there is a related class here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > Just with a little more clumsy interface. Except for ToString, > > GetWithDefaultIfUnset and operator==/!= you should be able to replace > Settable<> > > with Maybe<>. Or vice versa. > > With this sort of class, it's all about the interface, since the implementation > is dead simple. So I'd feel uncomfortable using Settable because its > deliberately specialized, if only in name. Using Maybe in places that currently > use Settable would make sense, though. Yes.
andrew@webrtc.org changed reviewers: + mgraczyk@chromium.org
Nice! The test is really nifty. Just one minor comment. Adding Michael to this CL as well, as he will likely have useful feedback. Agreed, it would be good to see Maybe replace Settable in a follow-up CL. https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h File webrtc/base/maybe.h (right): https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:43: // TODO(kwiberg): =default the move constructor when MSVC supports it. Ugh :( https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:101: return m1.has_value_ && m2.has_value_ ? m1.value_ != m2.value_ return !(m1 == m2) is more typical?
Mostly minor comments, but conversion from T should either be allowed or a comment should be added since the behavior deviates from optional. https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h File webrtc/base/maybe.h (right): https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:27: // A moved-from Maybe<T> may only be destroyed. Specifically, you may not Why? I can't do this? Maybe<T> x{10}; func(std::move(x)); x = 20; .... use x again ... Normally the rule is that moved-from values are left in a valid but unspecified state. That implies that they can be "re-used" by reassigning to them. https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:39: explicit Maybe(const T& val) : value_(val), has_value_(true) {} These constructors are not explicit for std::experimental::optional. This causes the following code to work with optional, but not with Maybe: Maybe<int> maybe = 1; Not sure whether or not you want that to work. https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:56: Maybe& operator=(const T& val) { std::optional does not have this overload. http://en.cppreference.com/w/cpp/experimental/optional/operator%3D I'm not sure why, because it does have the corresponding constructor. https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:67: friend void swap(Maybe& m1, Maybe& m2) { This trick might not work with some versions of the standard libraries that don't use ADL. http://stackoverflow.com/questions/11562/how-to-overload-stdswap#comment30080... It's probably okay if it builds on all of our targets. https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:92: } It would probably be worthwhile to include value_or already. Most code I have seen that uses optional also uses value_or. We could also include value() and use RTC_CHECK instead of DHECK in value(). That gives callers a "safe" way to access the value if they desire.
kwiberg@webrtc.org changed reviewers: + tommi@webrtc.org
https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h File webrtc/base/maybe.h (right): https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:27: // A moved-from Maybe<T> may only be destroyed. Specifically, you may not On 2015/10/20 02:44:44, mgraczyk wrote: > Why? I can't do this? > > Maybe<T> x{10}; > func(std::move(x)); > > x = 20; > > .... use x again ... > > > Normally the rule is that moved-from values are left in a valid but unspecified > state. That implies that they can be "re-used" by reassigning to them. Yeah, that's actually the case here too. I'll fix the wording. (The current wording comes from an earlier attempt to simply make moved-from Maybe<T> not contain a value, but that complicated the implementation too much.) https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:39: explicit Maybe(const T& val) : value_(val), has_value_(true) {} On 2015/10/20 02:44:44, mgraczyk wrote: > These constructors are not explicit for std::experimental::optional. > > This causes the following code to work with optional, but not with Maybe: > > Maybe<int> maybe = 1; > > Not sure whether or not you want that to work. I want it to work, but it means more style guide violations. My opinion is that this sort of class is precisely where bending the style guide rules makes sense, but I'd like more reviewers to weigh in. Fredrik already argued for not making these implicit in an earlier comment. https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:43: // TODO(kwiberg): =default the move constructor when MSVC supports it. On 2015/10/20 01:54:30, Andrew MacDonald wrote: > Ugh :( Yeah. C++11 says you're allowed to =default them, and it works in gcc and clang... https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:56: Maybe& operator=(const T& val) { On 2015/10/20 02:44:44, mgraczyk wrote: > std::optional does not have this overload. > http://en.cppreference.com/w/cpp/experimental/optional/operator%3D > > I'm not sure why, because it does have the corresponding constructor. And since it will implicitly turn T into std::optional<T>, assigning a T to a std::optional<T> would still work, right? https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:67: friend void swap(Maybe& m1, Maybe& m2) { On 2015/10/20 02:44:44, mgraczyk wrote: > This trick might not work with some versions of the standard libraries that > don't use ADL. > > http://stackoverflow.com/questions/11562/how-to-overload-stdswap#comment30080... > > It's probably okay if it builds on all of our targets. This page says the standard library is supposed to use ADL: http://en.cppreference.com/w/cpp/concept/Swappable But maybe that's only true for C++11, or only true in theory... Anyway, I thought the reason why the recommendation isn't just to specialize std::swap for your type is that you can't do partial template specialization? In other words, that it's impossible to specialize std::swap for Maybe<T> without specifying a concrete T. https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:92: } On 2015/10/20 02:44:44, mgraczyk wrote: > It would probably be worthwhile to include value_or already. Most code I have > seen that uses optional also uses value_or. Sounds reasonable. But I don't understand why std::experimental::optional::value_or has such a complicated type; I was thinking of using T value_or(const T&) and T value_or(T&&). (IIRC, I can't do anything fancy if *this is an rvalue because Microsoft.) > We could also include value() and use RTC_CHECK instead of DHECK in value(). > That gives callers a "safe" way to access the value if they desire. Yes, if we have value_or(), value() makes sense too. https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:101: return m1.has_value_ && m2.has_value_ ? m1.value_ != m2.value_ On 2015/10/20 01:54:30, Andrew MacDonald wrote: > return !(m1 == m2) > > is more typical? Yes. But, if I do that, then Maybe<T>::operator!= is implemented in terms of T::operator==. This way, Maybe<T>::operator!= is implemented in terms of T::operator!=. I guess this isn't likely to matter for a whole lot of Ts, though, so would you still like me to change it?
tommi@webrtc.org changed reviewers: - tommi@webrtc.org
https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h File webrtc/base/maybe.h (right): https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:92: } On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > I was thinking of using T value_or(const T&) and T value_or(T&&). > (IIRC, I can't do anything fancy if *this is an rvalue because > Microsoft.) I ended up doing only const T& value_or(const T&) const. T&& value_or(T&&) && would have been useful (since callers could then move from the return value), but as I said before we can't use it (yet).
https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h File webrtc/base/maybe.h (right): https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:39: explicit Maybe(const T& val) : value_(val), has_value_(true) {} On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > On 2015/10/20 02:44:44, mgraczyk wrote: > > These constructors are not explicit for std::experimental::optional. > > > > This causes the following code to work with optional, but not with Maybe: > > > > Maybe<int> maybe = 1; > > > > Not sure whether or not you want that to work. > > I want it to work, but it means more style guide violations. My opinion is that > this sort of class is precisely where bending the style guide rules makes sense, > but I'd like more reviewers to weigh in. Fredrik already argued for not making > these implicit in an earlier comment. Personally, I'd prefer the implicit conversions. ArraySlice in google3 is an example of a utility class with the same feature. Since you're modelling Maybe on experimental::optional, I think it's least surprising if you follow its conventions. https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:56: Maybe& operator=(const T& val) { On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > On 2015/10/20 02:44:44, mgraczyk wrote: > > std::optional does not have this overload. > > http://en.cppreference.com/w/cpp/experimental/optional/operator%3D > > > > I'm not sure why, because it does have the corresponding constructor. > > And since it will implicitly turn T into std::optional<T>, assigning a T to a > std::optional<T> would still work, right? So if you remove explicit from Maybe's corresponding constructor, remove this operator as well? https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:101: return m1.has_value_ && m2.has_value_ ? m1.value_ != m2.value_ On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > On 2015/10/20 01:54:30, Andrew MacDonald wrote: > > return !(m1 == m2) > > > > is more typical? > > Yes. But, if I do that, then Maybe<T>::operator!= is implemented in terms of > T::operator==. This way, Maybe<T>::operator!= is implemented in terms of > T::operator!=. I guess this isn't likely to matter for a whole lot of Ts, > though, so would you still like me to change it? No, I see why you did it this way now. Thanks for the explanation.
lgtm. Nice tests. make_maybe would likely be helpful, but is not necessary. https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h File webrtc/base/maybe.h (right): https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:39: explicit Maybe(const T& val) : value_(val), has_value_(true) {} On 2015/10/20 18:38:04, Andrew MacDonald wrote: > On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > > On 2015/10/20 02:44:44, mgraczyk wrote: > > > These constructors are not explicit for std::experimental::optional. > > > > > > This causes the following code to work with optional, but not with Maybe: > > > > > > Maybe<int> maybe = 1; > > > > > > Not sure whether or not you want that to work. > > > > I want it to work, but it means more style guide violations. My opinion is > that > > this sort of class is precisely where bending the style guide rules makes > sense, > > but I'd like more reviewers to weigh in. Fredrik already argued for not making > > these implicit in an earlier comment. > > Personally, I'd prefer the implicit conversions. ArraySlice in google3 is an > example of a utility class with the same feature. > > Since you're modelling Maybe on experimental::optional, I think it's least > surprising if you follow its conventions. +1 https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:56: Maybe& operator=(const T& val) { On 2015/10/20 18:38:04, Andrew MacDonald wrote: > On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > > On 2015/10/20 02:44:44, mgraczyk wrote: > > > std::optional does not have this overload. > > > http://en.cppreference.com/w/cpp/experimental/optional/operator%3D > > > > > > I'm not sure why, because it does have the corresponding constructor. > > > > And since it will implicitly turn T into std::optional<T>, assigning a T to a > > std::optional<T> would still work, right? > > So if you remove explicit from Maybe's corresponding constructor, remove this > operator as well? Yes I believe this is not needed. https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:92: } On 2015/10/20 13:35:30, kwiberg-webrtc wrote: > On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > > I was thinking of using T value_or(const T&) and T value_or(T&&). > > (IIRC, I can't do anything fancy if *this is an rvalue because > > Microsoft.) > > I ended up doing only const T& value_or(const T&) const. T&& > value_or(T&&) && would have been useful (since callers could then move > from the return value), but as I said before we can't use it (yet). Yes, without std::move() and library support your version will be as useful as possible.
https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h File webrtc/base/maybe.h (right): https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:39: explicit Maybe(const T& val) : value_(val), has_value_(true) {} On 2015/10/20 19:10:23, mgraczyk wrote: > On 2015/10/20 18:38:04, Andrew MacDonald wrote: > > On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > > > On 2015/10/20 02:44:44, mgraczyk wrote: > > > > These constructors are not explicit for std::experimental::optional. > > > > > > > > This causes the following code to work with optional, but not with Maybe: > > > > > > > > Maybe<int> maybe = 1; > > > > > > > > Not sure whether or not you want that to work. > > > > > > I want it to work, but it means more style guide violations. My opinion is > > that > > > this sort of class is precisely where bending the style guide rules makes > > sense, > > > but I'd like more reviewers to weigh in. Fredrik already argued for not > making > > > these implicit in an earlier comment. > > > > Personally, I'd prefer the implicit conversions. ArraySlice in google3 is an > > example of a utility class with the same feature. > > > > Since you're modelling Maybe on experimental::optional, I think it's least > > surprising if you follow its conventions. > > +1 Swell. I'll restore the implicit conversions then. https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:56: Maybe& operator=(const T& val) { On 2015/10/20 19:10:23, mgraczyk wrote: > On 2015/10/20 18:38:04, Andrew MacDonald wrote: > > On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > > > On 2015/10/20 02:44:44, mgraczyk wrote: > > > > std::optional does not have this overload. > > > > http://en.cppreference.com/w/cpp/experimental/optional/operator%3D > > > > > > > > I'm not sure why, because it does have the corresponding constructor. > > > > > > And since it will implicitly turn T into std::optional<T>, assigning a T to > a > > > std::optional<T> would still work, right? > > > > So if you remove explicit from Maybe's corresponding constructor, remove this > > operator as well? > > Yes I believe this is not needed. I'll try it, and see if the test detects any changes in e.g. the number of constructor calls. Conceptually at least, implicit conversion before assignment means creating a temporary that didn't need creating with these assignment operators. https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:92: } On 2015/10/20 19:10:23, mgraczyk wrote: > On 2015/10/20 13:35:30, kwiberg-webrtc wrote: > > On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > > > I was thinking of using T value_or(const T&) and T value_or(T&&). > > > (IIRC, I can't do anything fancy if *this is an rvalue because > > > Microsoft.) > > > > I ended up doing only const T& value_or(const T&) const. T&& > > value_or(T&&) && would have been useful (since callers could then move > > from the return value), but as I said before we can't use it (yet). > > Yes, without std::move() and library support your version will be as useful as > possible. Well, you can have rvalues without std::move. Consider e.g. Maybe<X> GetX(); X x = GetX().value_or(X()); Both the Maybe and the default value are rvalues here, so T&& value_or(T&&) && would have been selected, allowing x to be move constructed. With just const T& value_or(const T&) const available, it'll be copy constructed instead. That said, value_or isn't likely to be useful with big types anyway, since you unconditionally pay for constructing the default value. I toyed with the idea of having it accept a functor that would create the default value on demand, but again it would be too complex. One of the best things about this implementation is how dead simple it is.
On 2015/10/20 19:10:23, mgraczyk wrote: > make_maybe would likely be helpful, but is not necessary. Hmm. How useful would it be, really, given implicit conversion from T to Maybe<T>?
https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h File webrtc/base/maybe.h (right): https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:56: Maybe& operator=(const T& val) { On 2015/10/20 21:11:59, kwiberg-webrtc wrote: > On 2015/10/20 19:10:23, mgraczyk wrote: > > On 2015/10/20 18:38:04, Andrew MacDonald wrote: > > > On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > > > > On 2015/10/20 02:44:44, mgraczyk wrote: > > > > > std::optional does not have this overload. > > > > > http://en.cppreference.com/w/cpp/experimental/optional/operator%3D > > > > > > > > > > I'm not sure why, because it does have the corresponding constructor. > > > > > > > > And since it will implicitly turn T into std::optional<T>, assigning a T > to > > a > > > > std::optional<T> would still work, right? > > > > > > So if you remove explicit from Maybe's corresponding constructor, remove > this > > > operator as well? > > > > Yes I believe this is not needed. > > I'll try it, and see if the test detects any changes in e.g. the number of > constructor calls. Conceptually at least, implicit conversion before assignment > means creating a temporary that didn't need creating with these assignment > operators. As expected, it works, but results in a temporary Maybe being created, moved from, and destroyed. I'll keep these unless you object.
> Hmm. How useful would it be, really, given implicit conversion from T to Maybe<T>? You're right, I can't think of a good use that isn't covered by implicit conversions.
https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h File webrtc/base/maybe.h (right): https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:56: Maybe& operator=(const T& val) { On 2015/10/20 21:26:00, kwiberg-webrtc wrote: > On 2015/10/20 21:11:59, kwiberg-webrtc wrote: > > On 2015/10/20 19:10:23, mgraczyk wrote: > > > On 2015/10/20 18:38:04, Andrew MacDonald wrote: > > > > On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > > > > > On 2015/10/20 02:44:44, mgraczyk wrote: > > > > > > std::optional does not have this overload. > > > > > > http://en.cppreference.com/w/cpp/experimental/optional/operator%3D > > > > > > > > > > > > I'm not sure why, because it does have the corresponding constructor. > > > > > > > > > > And since it will implicitly turn T into std::optional<T>, assigning a T > > to > > > a > > > > > std::optional<T> would still work, right? > > > > > > > > So if you remove explicit from Maybe's corresponding constructor, remove > > this > > > > operator as well? > > > > > > Yes I believe this is not needed. > > > > I'll try it, and see if the test detects any changes in e.g. the number of > > constructor calls. Conceptually at least, implicit conversion before > assignment > > means creating a temporary that didn't need creating with these assignment > > operators. > > As expected, it works, but results in a temporary Maybe being created, moved > from, and destroyed. I'll keep these unless you object. Yes but I would be surprised if the temporary were not elided with optimization. Does the test run with optimization and still show an extra temporary created? https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:67: friend void swap(Maybe& m1, Maybe& m2) { On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > On 2015/10/20 02:44:44, mgraczyk wrote: > > This trick might not work with some versions of the standard libraries that > > don't use ADL. > > > > > http://stackoverflow.com/questions/11562/how-to-overload-stdswap#comment30080... > > > > It's probably okay if it builds on all of our targets. > > This page says the standard library is supposed to use ADL: > http://en.cppreference.com/w/cpp/concept/Swappable > But maybe that's only true for C++11, or only true in theory... > > Anyway, I thought the reason why the recommendation isn't just to specialize > std::swap for your type is that you can't do partial template specialization? In > other words, that it's impossible to specialize std::swap for Maybe<T> without > specifying a concrete T. The alternative would be to use a non-friend, non-member with the same signature. Yours is option #2, the alternative is option #1 in the following http://en.cppreference.com/w/cpp/concept/Swappable I like yours better, and there is no problem if it works on all the compilers we care about. https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:92: } On 2015/10/20 21:11:59, kwiberg-webrtc wrote: > On 2015/10/20 19:10:23, mgraczyk wrote: > > On 2015/10/20 13:35:30, kwiberg-webrtc wrote: > > > On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > > > > I was thinking of using T value_or(const T&) and T value_or(T&&). > > > > (IIRC, I can't do anything fancy if *this is an rvalue because > > > > Microsoft.) > > > > > > I ended up doing only const T& value_or(const T&) const. T&& > > > value_or(T&&) && would have been useful (since callers could then move > > > from the return value), but as I said before we can't use it (yet). > > > > Yes, without std::move() and library support your version will be as useful as > > possible. > > Well, you can have rvalues without std::move. Consider e.g. > > Maybe<X> GetX(); > X x = GetX().value_or(X()); > > Both the Maybe and the default value are rvalues here, so T&& value_or(T&&) && > would have been selected, allowing x to be move constructed. With just const T& > value_or(const T&) const available, it'll be copy constructed instead. > > That said, value_or isn't likely to be useful with big types anyway, since you > unconditionally pay for constructing the default value. I toyed with the idea of > having it accept a functor that would create the default value on demand, but > again it would be too complex. One of the best things about this implementation > is how dead simple it is. I see and I agree that it is best to keep it simple.
https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h File webrtc/base/maybe.h (right): https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:56: Maybe& operator=(const T& val) { On 2015/10/20 22:16:19, mgraczyk wrote: > On 2015/10/20 21:26:00, kwiberg-webrtc wrote: > > On 2015/10/20 21:11:59, kwiberg-webrtc wrote: > > > On 2015/10/20 19:10:23, mgraczyk wrote: > > > > On 2015/10/20 18:38:04, Andrew MacDonald wrote: > > > > > On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > > > > > > On 2015/10/20 02:44:44, mgraczyk wrote: > > > > > > > std::optional does not have this overload. > > > > > > > http://en.cppreference.com/w/cpp/experimental/optional/operator%3D > > > > > > > > > > > > > > I'm not sure why, because it does have the corresponding > constructor. > > > > > > > > > > > > And since it will implicitly turn T into std::optional<T>, assigning a > T > > > to > > > > a > > > > > > std::optional<T> would still work, right? > > > > > > > > > > So if you remove explicit from Maybe's corresponding constructor, remove > > > this > > > > > operator as well? > > > > > > > > Yes I believe this is not needed. > > > > > > I'll try it, and see if the test detects any changes in e.g. the number of > > > constructor calls. Conceptually at least, implicit conversion before > > assignment > > > means creating a temporary that didn't need creating with these assignment > > > operators. > > > > As expected, it works, but results in a temporary Maybe being created, moved > > from, and destroyed. I'll keep these unless you object. > > Yes but I would be surprised if the temporary were not elided with optimization. > Does the test run with optimization and still show an extra temporary created? Yes, the extra temp is created even with optimizations enabled. Isn't that expected---this situation isn't one where copy elision is supposed to be allowed, is it? It's allowed for copy/move construction, not copy/move assignment. https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:67: friend void swap(Maybe& m1, Maybe& m2) { On 2015/10/20 22:16:19, mgraczyk wrote: > On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > > On 2015/10/20 02:44:44, mgraczyk wrote: > > > This trick might not work with some versions of the standard libraries that > > > don't use ADL. > > > > > > > > > http://stackoverflow.com/questions/11562/how-to-overload-stdswap#comment30080... > > > > > > It's probably okay if it builds on all of our targets. > > > > This page says the standard library is supposed to use ADL: > > http://en.cppreference.com/w/cpp/concept/Swappable > > But maybe that's only true for C++11, or only true in theory... > > > > Anyway, I thought the reason why the recommendation isn't just to specialize > > std::swap for your type is that you can't do partial template specialization? > In > > other words, that it's impossible to specialize std::swap for Maybe<T> without > > specifying a concrete T. > > The alternative would be to use a non-friend, non-member with the same > signature. Yours is option #2, the alternative is option #1 in the following > > http://en.cppreference.com/w/cpp/concept/Swappable > > I like yours better, and there is no problem if it works on all the compilers we > care about. I see. And yes, the unit test for swap proves that we call Maybe's own swap and not something else (since nothing else would be as efficient). But I'm curious: when would having a non-member swap in the enclosing namespace work, when the inline friend swap wouldn't?
https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h File webrtc/base/maybe.h (right): https://codereview.webrtc.org/1413763003/diff/40001/webrtc/base/maybe.h#newco... webrtc/base/maybe.h:67: friend void swap(Maybe& m1, Maybe& m2) { On 2015/10/20 22:57:18, kwiberg-webrtc wrote: > On 2015/10/20 22:16:19, mgraczyk wrote: > > On 2015/10/20 08:43:20, kwiberg-webrtc wrote: > > > On 2015/10/20 02:44:44, mgraczyk wrote: > > > > This trick might not work with some versions of the standard libraries > that > > > > don't use ADL. > > > > > > > > > > > > > > http://stackoverflow.com/questions/11562/how-to-overload-stdswap#comment30080... > > > > > > > > It's probably okay if it builds on all of our targets. > > > > > > This page says the standard library is supposed to use ADL: > > > http://en.cppreference.com/w/cpp/concept/Swappable > > > But maybe that's only true for C++11, or only true in theory... > > > > > > Anyway, I thought the reason why the recommendation isn't just to specialize > > > std::swap for your type is that you can't do partial template > specialization? > > In > > > other words, that it's impossible to specialize std::swap for Maybe<T> > without > > > specifying a concrete T. > > > > The alternative would be to use a non-friend, non-member with the same > > signature. Yours is option #2, the alternative is option #1 in the following > > > > http://en.cppreference.com/w/cpp/concept/Swappable > > > > I like yours better, and there is no problem if it works on all the compilers > we > > care about. > > I see. And yes, the unit test for swap proves that we call Maybe's own swap and > not something else (since nothing else would be as efficient). > > But I'm curious: when would having a non-member swap in the enclosing namespace > work, when the inline friend swap wouldn't? I can't think of any. From the SO answers I believe the problem was with non-conforming compilers rather than libraries as I originally thought.
lgtm
The CQ bit was checked by kwiberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from mgraczyk@chromium.org Link to the patchset: https://codereview.webrtc.org/1413763003/#ps140001 (title: "Make constructors implicit again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413763003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413763003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1339)
Message was sent while issue was closed.
Committed patchset #8 (id:140001) manually as 6e587200db7ddae0d33e5187b73acdd41175584c (presubmit successful).
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6e587200db7ddae0d33e5187b73acdd41175584c Cr-Commit-Position: refs/heads/master@{#10353} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
