Made rtc::Optional a bit more like std::optional.

Made rtc::Optional a bit more like std::optional.

Its value constructors are now implicit (i.e. const& T and T&&). std::optional in C++17 is constexpr, but this isn't possible yet.
Equality and inequality comparisons can now compare directly with values;
if the Optional is empty, it's not equal to any value.

[ossu, 2016-06-16 13:33:04]

Description was changed from

==========
Made rtc::Optional a bit more like std::optional.

Its value constructors are now implicit (i.e. const& T and T&&).
Equality and inequality comparisons can now compare directly with values;
if the Optional is empty, it's not equal to any value.
==========

to

==========
Made rtc::Optional a bit more like std::optional.

Its value constructors are now implicit (i.e. const& T and T&&). std::optional
in C++17 is constexpr, but this isn't possible yet.
Equality and inequality comparisons can now compare directly with values;
if the Optional is empty, it's not equal to any value.
==========

[ossu, 2016-06-16 13:33:04]

ossu@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[ossu, 2016-06-16 13:35:38]

Was using a rtc::Optional a bit in a different CL and noticed it was missing a
couple of useful features from std::optional. Implicit constructors make it
easier to return values from a function returning an optional and using optional
as function parameters. Comparison against values are useful in general.

[tommi, 2016-06-16 16:08:22]

(not adding myself as reviewer, just a question)

https://codereview.webrtc.org/2072713002/diff/1/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/2072713002/diff/1/webrtc/base/optional.h#newcode65
webrtc/base/optional.h:65: Optional(const T& value) : has_value_(true) {
this goes against the style guide though.  Is requiring explicit causing
problems?

[kwiberg-webrtc, 2016-06-16 17:26:43]

I agree that this CL makes Optional more useful, but...

Chromium has an Optional type nowadays, and theirs goes to great effort to be
std::optional-compatible. I think what we should do is steal that one, instead
of reimplementing the same thing ourselves.

Opinions?

https://codereview.webrtc.org/2072713002/diff/1/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/2072713002/diff/1/webrtc/base/optional.h#newcode65
webrtc/base/optional.h:65: Optional(const T& value) : has_value_(true) {
On 2016/06/16 16:08:22, tommi-webrtc wrote:
> this goes against the style guide though.  Is requiring explicit causing
> problems?

It's very convenient to have T convert implicitly to Optional<T>, and it's what
std::optional does. The standard library violates our style guide quite a lot...

[tommi, 2016-06-16 21:35:49]

On 2016/06/16 17:26:43, kwiberg-webrtc wrote:
> I agree that this CL makes Optional more useful, but...
> 
> Chromium has an Optional type nowadays, and theirs goes to great effort to be
> std::optional-compatible. I think what we should do is steal that one, instead
> of reimplementing the same thing ourselves.
> 
> Opinions?
> 
> https://codereview.webrtc.org/2072713002/diff/1/webrtc/base/optional.h
> File webrtc/base/optional.h (right):
> 
>
https://codereview.webrtc.org/2072713002/diff/1/webrtc/base/optional.h#newcode65
> webrtc/base/optional.h:65: Optional(const T& value) : has_value_(true) {
> On 2016/06/16 16:08:22, tommi-webrtc wrote:
> > this goes against the style guide though.  Is requiring explicit causing
> > problems?
> 
> It's very convenient to have T convert implicitly to Optional<T>, and it's
what
> std::optional does. The standard library violates our style guide quite a
lot...

Yup, no question.  What I was thinking is that this is still code that we're
writing and explicit is compatible with non-explicit but not the other way
around.

[kwiberg-webrtc, 2016-06-17 07:17:37]

On 2016/06/16 21:35:49, tommi-webrtc wrote:
> On 2016/06/16 17:26:43, kwiberg-webrtc wrote:
> > I agree that this CL makes Optional more useful, but...
> > 
> > Chromium has an Optional type nowadays, and theirs goes to great effort to
be
> > std::optional-compatible. I think what we should do is steal that one,
instead
> > of reimplementing the same thing ourselves.
> > 
> > Opinions?
> > 
> > https://codereview.webrtc.org/2072713002/diff/1/webrtc/base/optional.h
> > File webrtc/base/optional.h (right):
> > 
> >
>
https://codereview.webrtc.org/2072713002/diff/1/webrtc/base/optional.h#newcode65
> > webrtc/base/optional.h:65: Optional(const T& value) : has_value_(true) {
> > On 2016/06/16 16:08:22, tommi-webrtc wrote:
> > > this goes against the style guide though.  Is requiring explicit causing
> > > problems?
> > 
> > It's very convenient to have T convert implicitly to Optional<T>, and it's
> what
> > std::optional does. The standard library violates our style guide quite a
> lot...
> 
> Yup, no question.  What I was thinking is that this is still code that we're
> writing and explicit is compatible with non-explicit but not the other way
> around.

Yes. And I'd argue that non-explicit makes sense in this case, as a style guide
exception. pthatcher@ argued the opposite, which is why rtc::Optional currently
doesn't have non-explicit constructors.

If we pulled in Chromium's Optional, there would be a very strong argument in
favor of non-explicit, since that Optional implementation tries to imitate
std::optional pretty much exactly.

[ossu, 2016-06-17 07:58:18]

On 2016/06/17 07:17:37, kwiberg-webrtc wrote:
> On 2016/06/16 21:35:49, tommi-webrtc wrote:
> > On 2016/06/16 17:26:43, kwiberg-webrtc wrote:
> > > I agree that this CL makes Optional more useful, but...
> > > 
> > > Chromium has an Optional type nowadays, and theirs goes to great effort to
> be
> > > std::optional-compatible. I think what we should do is steal that one,
> instead
> > > of reimplementing the same thing ourselves.
> > > 
> > > Opinions?

I'll happily pull in Chromium's Optional if it's like std::optional. I foresee
us eventually (maybe a couple of years from now) going to std::optional like we
did with std::unique_ptr, so it's good if our code is conforming to the standard
interface as closely as possible.

> > > On 2016/06/16 16:08:22, tommi-webrtc wrote:
> > > > this goes against the style guide though.  Is requiring explicit causing
> > > > problems?

It's not the end of the world, but it will make using Optional less tempting to
use, over, say, sentinel values.
It's much more of a hassle to write:
  return rtc::Optional<int>(5);
than
  return 5;

Especially since there are no risks with implicit conversions in this case: an
int _is_ for all intents and purposes also an Optional<int>. The converse is, of
course, not true, which is why there's no implicit type-conversion operator on
it.

> If we pulled in Chromium's Optional, there would be a very strong argument in
> favor of non-explicit, since that Optional implementation tries to imitate
> std::optional pretty much exactly.

I vote for doing just that. Do you know if it's also got the equivalent of
nullopt? That'd make working with no-value Optionals easier as well.

[kwiberg-webrtc, 2016-06-17 08:14:31]

On 2016/06/17 07:58:18, ossu wrote:
> On 2016/06/17 07:17:37, kwiberg-webrtc wrote:
> > On 2016/06/16 21:35:49, tommi-webrtc wrote:
> > > On 2016/06/16 17:26:43, kwiberg-webrtc wrote:
> > > > I agree that this CL makes Optional more useful, but...
> > > > 
> > > > Chromium has an Optional type nowadays, and theirs goes to great effort
to
> > be
> > > > std::optional-compatible. I think what we should do is steal that one,
> > instead
> > > > of reimplementing the same thing ourselves.
> > > > 
> > > > Opinions?
> 
> I'll happily pull in Chromium's Optional if it's like std::optional. I foresee
> us eventually (maybe a couple of years from now) going to std::optional like
we
> did with std::unique_ptr, so it's good if our code is conforming to the
standard
> interface as closely as possible.

+1

but given our timeline for C++11 and C++14 adoption, I doubt it will happen this
decade.

> > > > On 2016/06/16 16:08:22, tommi-webrtc wrote:
> > > > > this goes against the style guide though.  Is requiring explicit
causing
> > > > > problems?
> 
> It's not the end of the world, but it will make using Optional less tempting
to
> use, over, say, sentinel values.
> It's much more of a hassle to write:
>   return rtc::Optional<int>(5);
> than
>   return 5;
> 
> Especially since there are no risks with implicit conversions in this case: an
> int _is_ for all intents and purposes also an Optional<int>. The converse is,
of
> course, not true, which is why there's no implicit type-conversion operator on
> it.
> 
> > If we pulled in Chromium's Optional, there would be a very strong argument
in
> > favor of non-explicit, since that Optional implementation tries to imitate
> > std::optional pretty much exactly.
> 
> I vote for doing just that. Do you know if it's also got the equivalent of
> nullopt? That'd make working with no-value Optionals easier as well.

I'm pretty sure it does.

[ossu, 2016-06-17 14:16:52]

Alright, I've put up a separate CL importing a slightly modified version of
Optional from Chromium here:
https://codereview.webrtc.org/2071003003/

I'll close this CL when/if that one lands.

[tommi, 2016-06-17 15:10:36]

On 2016/06/17 14:16:52, ossu wrote:
> Alright, I've put up a separate CL importing a slightly modified version of
> Optional from Chromium here:
> https://codereview.webrtc.org/2071003003/
> 
> I'll close this CL when/if that one lands.

ok, plan sgtm.

[ossu, 2017-02-21 16:57:49]

On 2016/06/17 15:10:36, tommi (webrtc) wrote:
> On 2016/06/17 14:16:52, ossu wrote:
> > Alright, I've put up a separate CL importing a slightly modified version of
> > Optional from Chromium here:
> > https://codereview.webrtc.org/2071003003/
> > 
> > I'll close this CL when/if that one lands.
> 
> ok, plan sgtm.

I'll close this one now. I'll look at following up the CL above with a more
recent version of Optional.