Rename Maybe to Optional

Rename Maybe to Optional

And add examples of good and bad usage to the documentation.

R=aluebs@webrtc.org, henrik.lundin@webrtc.org, pthatcher@webrtc.org

Committed: https://crrev.com/be57983f4bd875c39a229bab5112b32dad004057
Cr-Commit-Position: refs/heads/master@{#10588}

[kwiberg-webrtc, 2015-11-10 09:30:05]

Description was changed from

==========
Rename Maybe to Optional

And add examples of good and bad usage to the documentation.
==========

to

==========
Rename Maybe to Optional

And add examples of good and bad usage to the documentation.
==========

[kwiberg-webrtc, 2015-11-10 09:30:05]

kwiberg@webrtc.org changed reviewers:
+ aluebs@webrtc.org, henrik.lundin@webrtc.org, pthatcher@webrtc.org

[kwiberg-webrtc, 2015-11-10 09:32:42]

pthatcher: talk/ and webrtc/base/
aluebs: webrtc/modules/audio_processing/
hlundin: webrtc/modules/audio_coding/

And everyone's opinion on the doc additions in webrtc/base/optional.h.

[hlundin-webrtc, 2015-11-10 09:59:51]

webrtc/modules/audio_coding/ and doc text: lgtm

[aluebs-webrtc, 2015-11-10 17:15:26]

modules/audio_processing and doc lgtm

[pthatcher1, 2015-11-10 19:07:53]

lgtm to the renaming, but I do worry about the settability of Maybe/Optional as
compared to std::experimental::optional and the previous Settable.

https://codereview.webrtc.org/1432553007/diff/40001/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/1432553007/diff/40001/webrtc/base/optional.h#ne...
webrtc/base/optional.h:70: : value_(static_cast<T&&>(val)), has_value_(true) {}
I know this is not related to a rename, but it seems pretty important that we
have a clear way to set the value of an Optional even if it hasn't been set
before.  For example:

Optional<int> foo; 
*foo = 5;

currently blows up.  And 

Optional<int> foo; 
foo = 5;

is not supported.


But it looks like the latter is supported in std::experimental::optional. 
Without either of these, it looks like we have to:

Optional<int> foo; 
foo = Optional<int>(5);

, which is ugly.



If we can't get "foo = 5", can we at least add a method that's "foo.Set(5)"?

[noahric, 2015-11-10 19:29:55]

noahric@chromium.org changed reviewers:
+ noahric@chromium.org

[noahric, 2015-11-10 19:29:56]

https://codereview.webrtc.org/1432553007/diff/40001/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/1432553007/diff/40001/webrtc/base/optional.h#ne...
webrtc/base/optional.h:70: : value_(static_cast<T&&>(val)), has_value_(true) {}
Ditto. I was just updating some other code to match the switchover for
audio/video options, and the semantics here are hard, especially given that
peter's first example (setting an unset Maybe) may work sometimes, depending on
the state of the option before then. So you could write e.g. webrtcvideoengine
to take options, use the wrong version, and it would work as long as all (known)
callers set it something when setting options..

Also, I find the operator boolean scary. It makes Maybe<bool> dangerous, since
it's easy to miss c/*c in if statements and it "works" if you have it set to
true (like you wouldn't notice it if you default the value to true and then only
test the defaults).

I know this is to make things follow std::optional, but the new API seems more
prone to error :(

[kwiberg-webrtc, 2015-11-10 19:36:17]

https://codereview.webrtc.org/1432553007/diff/40001/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/1432553007/diff/40001/webrtc/base/optional.h#ne...
webrtc/base/optional.h:70: : value_(static_cast<T&&>(val)), has_value_(true) {}
On 2015/11/10 19:07:53, pthatcher1 wrote:
> I know this is not related to a rename, but it seems pretty important that we
> have a clear way to set the value of an Optional even if it hasn't been set
> before.  For example:
> 
> Optional<int> foo; 
> *foo = 5;
> 
> currently blows up.

Yes, by construction; prefix * and = are two separate operations, so this
inherently dereferences first and assigns second, which only works if the
Optional was already set. I don't think this can be made to work; at least I
can't see how.

> And 
> 
> Optional<int> foo; 
> foo = 5;
> 
> is not supported. But it looks like the latter is supported in
std::experimental::optional. 
> Without either of these, it looks like we have to:
> 
> Optional<int> foo; 
> foo = Optional<int>(5);
> 
> , which is ugly.

An earlier version of the Maybe type supported assignments from the value type
(indeed, implicit conversion from T to Maybe<T>), but solenberg@ argued
(convincingly) that this makes it hard to see that the thing being assigned to
is an Optional. (He was looking at the CL that replaces Settable with Maybe when
he thought of this.) The plan we came up with is to start out with the current
version, and see if all the explicit Optional<T>()s in the code feel like a net
gain or net loss. (The opposite, starting with the implicit conversions and
maybe removing them later, would be much more troublesome if the type is
successful.)

> If we can't get "foo = 5", can we at least add a method that's "foo.Set(5)"?

We could, but that'd be incompatible with std::experimental::optional. I'd
rather stick with the plan above. My current preference is to bring back the
implicit conversion from T to Optional<T>, but it's probably worth it to wait
and see a bit.

[pthatcher1, 2015-11-10 20:21:38]

https://codereview.webrtc.org/1432553007/diff/40001/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/1432553007/diff/40001/webrtc/base/optional.h#ne...
webrtc/base/optional.h:70: : value_(static_cast<T&&>(val)), has_value_(true) {}
On 2015/11/10 19:36:17, kwiberg-webrtc wrote:
> On 2015/11/10 19:07:53, pthatcher1 wrote:
> > I know this is not related to a rename, but it seems pretty important that
we
> > have a clear way to set the value of an Optional even if it hasn't been set
> > before.  For example:
> > 
> > Optional<int> foo; 
> > *foo = 5;
> > 
> > currently blows up.
> 
> Yes, by construction; prefix * and = are two separate operations, so this
> inherently dereferences first and assigns second, which only works if the
> Optional was already set. I don't think this can be made to work; at least I
> can't see how.

We could just remove the "RTC_DCHECK(has_value_)" in T& operator*(), which would
make the release build and debug build behavior equivalent.  If you "*foo" when
you didn't assign a value, you get what you asked for.  Or we could add a safe
"bool Get(T*)" method like Settable had.

> 
> > And 
> > 
> > Optional<int> foo; 
> > foo = 5;
> > 
> > is not supported. But it looks like the latter is supported in
> std::experimental::optional. 
> > Without either of these, it looks like we have to:
> > 
> > Optional<int> foo; 
> > foo = Optional<int>(5);
> > 
> > , which is ugly.
> 
> An earlier version of the Maybe type supported assignments from the value type
> (indeed, implicit conversion from T to Maybe<T>), but solenberg@ argued
> (convincingly) that this makes it hard to see that the thing being assigned to
> is an Optional. (He was looking at the CL that replaces Settable with Maybe
when
> he thought of this.) The plan we came up with is to start out with the current
> version, and see if all the explicit Optional<T>()s in the code feel like a
net
> gain or net loss. (The opposite, starting with the implicit conversions and
> maybe removing them later, would be much more troublesome if the type is
> successful.)

> 
> > If we can't get "foo = 5", can we at least add a method that's "foo.Set(5)"?
> 
> We could, but that'd be incompatible with std::experimental::optional. I'd
> rather stick with the plan above. My current preference is to bring back the
> implicit conversion from T to Optional<T>, but it's probably worth it to wait
> and see a bit.

We can claim we shouldn't support "foo = 5" or we can claim we must be
compatible with "std::experimental::optional", but we can't claim both, because
if I'm reading this right
(http://en.cppreference.com/w/cpp/experimental/optional/operator%3D), there is
an assignment operator that allows a "bare" value, and the example at the bottom
even uses it.  

So, we need to pick whether we'll be compatible and support "foo = 5" or be
incompatible, in which case we can have our own "foo.Set(5)". 


I'm in favor of being incompatible.  It's experimental for a reason: it's likely
to change or disappear.  Plus, I think a safe Get method is better than
"if(foo){ *foo }" and a clear Set method is safer and more clear than "if(foo){
*foo 5 }else{foo = Optional<T>(5)".  But if we really do want to be compatible
with an experimental thing, at least be compatible enough to make "foo = 5"
work.

[kwiberg-webrtc, 2015-11-10 20:33:10]

https://codereview.webrtc.org/1432553007/diff/40001/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/1432553007/diff/40001/webrtc/base/optional.h#ne...
webrtc/base/optional.h:70: : value_(static_cast<T&&>(val)), has_value_(true) {}
On 2015/11/10 19:29:56, noahric wrote:
> Ditto. I was just updating some other code to match the switchover for
> audio/video options, and the semantics here are hard, especially given that
> peter's first example (setting an unset Maybe) may work sometimes, depending
on
> the state of the option before then. So you could write e.g. webrtcvideoengine
> to take options, use the wrong version, and it would work as long as all
(known)
> callers set it something when setting options..

Yeah, using prefix * without first checking if the Optional contains a value is
the main thing that will trip you up. I guess the main argument for the design
is that it works (and looks) just like a pointer. At least the implementation
has a DCHECK to catch this.

Unfortunately, you can't avoid this problem if you have some operator or method
that returns a non-const reference to the contained value. IIRC Settable didn't
have that, but that would have made it unsuitable for types that you typically
want to mutate in-place (such as any class with non-const methods).

> Also, I find the operator boolean scary. It makes Maybe<bool> dangerous, since
> it's easy to miss c/*c in if statements and it "works" if you have it set to
> true (like you wouldn't notice it if you default the value to true and then
only
> test the defaults).

Yes, Optional<bool> is a bit scary that way. A named method for testing if it's
set would have been safer.

> I know this is to make things follow std::optional, but the new API seems more
> prone to error :(

Presumably, yes. My guess is that the price is worth paying, but I may of course
be wrong.

[kwiberg-webrtc, 2015-11-10 21:31:31]

https://codereview.webrtc.org/1432553007/diff/40001/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/1432553007/diff/40001/webrtc/base/optional.h#ne...
webrtc/base/optional.h:70: : value_(static_cast<T&&>(val)), has_value_(true) {}
On 2015/11/10 20:21:38, pthatcher1 wrote:

> We could just remove the "RTC_DCHECK(has_value_)" in T& operator*(),
> which would make the release build and debug build behavior
> equivalent.  If you "*foo" when you didn't assign a value, you get
> what you asked for.

Yes, but that means that *x = 17 will work with our implementation,
but fail at runtime for std::experimental::optional. Bad idea if we
want to stay compatible; we want our implementation to have a subset
of the functionality, in that case.

> We can claim we shouldn't support "foo = 5" or we can claim we must
> be compatible with "std::experimental::optional", but we can't claim
> both, because if I'm reading this right
> (http://en.cppreference.com/w/cpp/experimental/optional/operator%3D),
> there is an assignment operator that allows a "bare" value, and the
> example at the bottom even uses it.
> 
> So, we need to pick whether we'll be compatible and support "foo =
> 5" or be incompatible, in which case we can have our own
> "foo.Set(5)".

We're currently compatible in the sense that we could replace every
occurrence of Optional with std::experimental::optional, and
everything would still work the same. In other words, Optional is
(almost) a subset of std::experimental::optional.

> I'm in favor of being incompatible.  It's experimental for a reason:
> it's likely to change or disappear.  Plus, I think a safe Get method
> is better than "if(foo){ *foo }"

I have an idea for an even safer one, but I'm a Compatibilist, and I'd
hate to provide ammunition for the Incompatibilist propaganda
machine. :-)

> and a clear Set method is safer and
> more clear than "if(foo){ *foo 5 }else{foo = Optional<T>(5)".

Oh, you don't have to do that. Just

  foo = Optional<T>(5);

will do. It's verbose, but quite safe.

> But if we really do want to be compatible with an experimental
> thing, at least be compatible enough to make "foo = 5" work.

Fair enough.

[kwiberg-webrtc, 2015-11-10 21:34:40]

Committed patchset #4 (id:60001) manually as
be57983f4bd875c39a229bab5112b32dad004057 (presubmit successful).

[commit-bot: I haz the power, 2015-11-10 21:34:43]

Patchset 4 (id:??) landed as
https://crrev.com/be57983f4bd875c39a229bab5112b32dad004057
Cr-Commit-Position: refs/heads/master@{#10588}

[noahric, 2015-11-10 22:17:05]

To wax philosophical for a minute:

I think the terms compatible/incompatible here are a misnomer. What you're
actually arguing is that the usage is close enough to <some other type> that we
should use it. If you're going to use that argument, why not scoped_ptr? It's
actually based on a non-experimental standardized thing (unique_ptr) and has
roughly the same drawbacks (it's undefined to access it if it isn't there; it'll
auto cast to bool in if expressions for if it is set or not). And it matches the
settings usage, other than a move constructor being able to steal it :) So if
the best counter argument to that is it should be std::shared_ptr and we don't
have a great analog for that, then I'd argue we should get rid of Optional<T>
and instead add a shared_ptr<T> :)

Looking at it another way, the specific things I'm arguing about were some of
the reasons why Settable<T> was introduced in the first place. Peter and I
discussed it also for quite awhile back when he introduced Settable, and the API
was picked specifically to have a "pit of success", meaning the obvious API was
the correct/safe API. Trying to measure that objectively, you can say "the most
reasonable API that requires the fewest characters to type and subsequently
compiles is the right API", with some hand-waving about "reasonable". In that
case, both the operator bool() and *foo = val fail that test, because the
unsafe/incorrect version requires less typing and generally appears reasonable.

(I know that sounds like a weird measurement, but I think it actually tracks how
much of development gets done; people write the simplest thing that
compiles/runs, and then pay for the bugs later)

> Oh, you don't have to do that. Just
> 
>   foo = Optional<T>(5);
> 
> will do. It's verbose, but quite safe.
> 

If the overall goal is to be a subset, then why not just make *foo = 5 illegal?
It'll still pass the "code compiled with rtc::Maybe will work with
std::optional" test, and it forces the safer usage.

[kwiberg-webrtc, 2015-11-11 00:59:46]

On 2015/11/10 22:17:05, noahric wrote:
> To wax philosophical for a minute:
> 
> I think the terms compatible/incompatible here are a misnomer. What you're
> actually arguing is that the usage is close enough to <some other type> that
we
> should use it. If you're going to use that argument, why not scoped_ptr? It's
> actually based on a non-experimental standardized thing (unique_ptr) and has
> roughly the same drawbacks (it's undefined to access it if it isn't there;
it'll
> auto cast to bool in if expressions for if it is set or not). And it matches
the
> settings usage, other than a move constructor being able to steal it :) So if
> the best counter argument to that is it should be std::shared_ptr and we don't
> have a great analog for that, then I'd argue we should get rid of Optional<T>
> and instead add a shared_ptr<T> :)

scoped_ptr/unique_ptr/shared_ptr allocate on the heap, which isn't always
desirable; scoped_ptr<int> is much more expensive than Optional<int>. Otherwise
I'm with you; std::optional's interface is similar enough to them that that must
have been one of the main design goals.

> Looking at it another way, the specific things I'm arguing about were some of
> the reasons why Settable<T> was introduced in the first place. Peter and I
> discussed it also for quite awhile back when he introduced Settable, and the
API
> was picked specifically to have a "pit of success", meaning the obvious API
was
> the correct/safe API. Trying to measure that objectively, you can say "the
most
> reasonable API that requires the fewest characters to type and subsequently
> compiles is the right API", with some hand-waving about "reasonable". In that
> case, both the operator bool() and *foo = val fail that test, because the
> unsafe/incorrect version requires less typing and generally appears
reasonable.
> 
> (I know that sounds like a weird measurement, but I think it actually tracks
how
> much of development gets done; people write the simplest thing that
> compiles/runs, and then pay for the bugs later)

I agree in general.

> > Oh, you don't have to do that. Just
> > 
> >   foo = Optional<T>(5);
> > 
> > will do. It's verbose, but quite safe.
> > 
> 
> If the overall goal is to be a subset, then why not just make *foo = 5
illegal?
> It'll still pass the "code compiled with rtc::Maybe will work with
> std::optional" test, and it forces the safer usage.

Make it illegal how? Change * to return a const reference?

[noahric, 2015-11-11 01:08:17]

(const reference) Yup, that's basically what I was thinking. Though the
compilation error for that may be equally confusing. So more
pit-of-successful-y, but still a non-obvious API.

The full context of this is that I was adapting some code to the new maybe.h and
it took cs.chromium.org spelunking to figure out how to set it, because the
header file is really dense, and then I did it wrong anyways; I used *foo, which
luckily a unit test caught, before I realized why it was wrong. And I went back
through when I was finished to find I made the other kind of mistake, and had
mixed up a foo/*foo usage for a Maybe<bool>.

A reasonable counterargument to all that would be "be a better programmer", and
I have no answer for that :)

[kwiberg-webrtc, 2015-11-11 01:49:30]

On 2015/11/11 01:08:17, noahric wrote:
> A reasonable counterargument to all that would be "be a better programmer",
and
> I have no answer for that :)

You're in a better position than I to see what about Optional is difficult to
learn, so that's valuable evidence.