Replace rtc::cricket::Settable with rtc::Maybe

Replace rtc::cricket::Settable with rtc::Maybe

The former is very similar to the latter, but less general (mostly in
naming).

This CL, which is the first to use Maybe at scale, also removes the implicit conversion from T to Maybe<T>, since it was agreed that the increased verbosity increased legibility.

Committed: https://crrev.com/102c6a61bc0b42dc0956d013530fc0213b7e881b
Cr-Commit-Position: refs/heads/master@{#10461}

[kwiberg-webrtc, 2015-10-28 10:19:58]

Patchset #1 (id:1) has been deleted

[kwiberg-webrtc, 2015-10-28 12:26:44]

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

[kwiberg-webrtc, 2015-10-28 12:26:45]

Fredrik, please take a look. Does it look like you imagined?

(I know I'll have to run this past an OWNER too, but I thought I'd let you
review it first.)

[the sun, 2015-10-28 13:18:01]

Generally looks really nice. A good improvement in readability.

My biggest concern being what I perceive as an inconsistency in syntax between
reading and writing the value of a Maybe<>. See comment in webrtcsession.cc.

https://codereview.webrtc.org/1430433004/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1430433004/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:689:
video_options_.unsignalled_recv_stream_limit =
The above two lines are pretty confusing.

This syntax is likely the reason std::optional isn't standardized yet. My
problem is that it is easy to forget that we're accessing the Maybe<> and not a
value, yet the second line looks like we have direct access to the value. 

Also it is inconsistent with standard C syntax in that you can use direct
assignment to set the value, but need to dereference to get the value.

https://codereview.webrtc.org/1430433004/diff/20001/talk/media/base/mediachan...
File talk/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1430433004/diff/20001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:226: if (o)
Again, I'm worried about the syntax... the implicit conversion to bool is
non-intuitive to read.

nit: other functions in this file (and libjingle) use braces for on-liners.

https://codereview.webrtc.org/1430433004/diff/20001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1430433004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:663: bool echo_cancellation = false;
You can move this into the conditional below, right? Or use
*options.echo_cancellation throughout.

https://codereview.webrtc.org/1430433004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:713: 0 &&
did git cl format do this?

https://codereview.webrtc.org/1430433004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:837: delay_agnostic_aec_ =
options.delay_agnostic_aec;
nit: brackets, and below

[kwiberg-webrtc, 2015-10-28 14:34:38]

https://codereview.webrtc.org/1430433004/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1430433004/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:689:
video_options_.unsignalled_recv_stream_limit =
On 2015/10/28 13:18:01, the sun wrote:
> The above two lines are pretty confusing.
> 
> This syntax is likely the reason std::optional isn't standardized yet. My
> problem is that it is easy to forget that we're accessing the Maybe<> and not
a
> value, yet the second line looks like we have direct access to the value. 
> 
> Also it is inconsistent with standard C syntax in that you can use direct
> assignment to set the value, but need to dereference to get the value.

I agree that it isn't 100% obvious. But this is how std::experimental::optional
does it, so I'm not sure how incompatible we ought to be.

Our options are basically (1) the current operator= overload:

  x = 4;

(2) a Set method (which is what Settable used):

  x.Set(4);

and (3) relying on operator= with a Maybe on the right-hand side:

  x = rtc::Maybe<int>(4);

If we don't pick option (1), my vote is on option (3), since that's compatible
with std::experimental::optional. What do you think?

https://codereview.webrtc.org/1430433004/diff/20001/talk/media/base/mediachan...
File talk/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1430433004/diff/20001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:226: if (o)
On 2015/10/28 13:18:01, the sun wrote:
> Again, I'm worried about the syntax... the implicit conversion to bool is
> non-intuitive to read.

Intrinsically nonintuitive, or just in combination with direct assignment from
the value type?

> nit: other functions in this file (and libjingle) use braces for on-liners.

Done.

https://codereview.webrtc.org/1430433004/diff/20001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1430433004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:663: bool echo_cancellation = false;
On 2015/10/28 13:18:01, the sun wrote:
> You can move this into the conditional below, right? Or use
> *options.echo_cancellation throughout.

Done.

https://codereview.webrtc.org/1430433004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:713: 0 &&
On 2015/10/28 13:18:01, the sun wrote:
> did git cl format do this?

Yes. Interesting.

https://codereview.webrtc.org/1430433004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:837: delay_agnostic_aec_ =
options.delay_agnostic_aec;
On 2015/10/28 13:18:01, the sun wrote:
> nit: brackets, and below

Done.

[the sun, 2015-10-28 14:45:48]

https://codereview.webrtc.org/1430433004/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1430433004/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:689:
video_options_.unsignalled_recv_stream_limit =
On 2015/10/28 14:34:38, kwiberg-webrtc wrote:
> On 2015/10/28 13:18:01, the sun wrote:
> > The above two lines are pretty confusing.
> > 
> > This syntax is likely the reason std::optional isn't standardized yet. My
> > problem is that it is easy to forget that we're accessing the Maybe<> and
not
> a
> > value, yet the second line looks like we have direct access to the value. 
> > 
> > Also it is inconsistent with standard C syntax in that you can use direct
> > assignment to set the value, but need to dereference to get the value.
> 
> I agree that it isn't 100% obvious. But this is how
std::experimental::optional
> does it, so I'm not sure how incompatible we ought to be.
> 
> Our options are basically (1) the current operator= overload:
> 
>   x = 4;
> 
> (2) a Set method (which is what Settable used):
> 
>   x.Set(4);
> 
> and (3) relying on operator= with a Maybe on the right-hand side:
> 
>   x = rtc::Maybe<int>(4);
> 
> If we don't pick option (1), my vote is on option (3), since that's compatible
> with std::experimental::optional. What do you think?

Option 4:
Drop assignment overload and always require dereference to access the value:

*x = 4

Which is a syntax which will work regardless?

[the sun, 2015-10-29 12:27:43]

Nice! LGTM

https://codereview.webrtc.org/1430433004/diff/80001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1430433004/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:689:
video_options_.unsignalled_recv_stream_limit = rtc::Maybe<int>(
to-o-tal nit: it would be slightly more readable if the rtc::Maybe<... started
on the next line.

[kwiberg-webrtc, 2015-10-29 12:35:01]

https://codereview.webrtc.org/1430433004/diff/80001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1430433004/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:689:
video_options_.unsignalled_recv_stream_limit = rtc::Maybe<int>(
On 2015/10/29 12:27:43, the sun wrote:
> to-o-tal nit: it would be slightly more readable if the rtc::Maybe<... started
> on the next line.

As you may have guessed, I've delegated the line wrapping to clang-format. I'd
rather not try to fight it...

[kwiberg-webrtc, 2015-10-29 12:38:06]

Description was changed from

==========
Replace rtc::cricket::Settable with rtc::Maybe

The former is very similar to the latter, but less general (mostly in
naming).
==========

to

==========
Replace rtc::cricket::Settable with rtc::Maybe

The former is very similar to the latter, but less general (mostly in
naming).
==========

[kwiberg-webrtc, 2015-10-29 12:38:06]

kwiberg@webrtc.org changed reviewers:
+ andrew@webrtc.org, tommi@webrtc.org

[kwiberg-webrtc, 2015-10-29 12:42:18]

Now that solenberg@ likes it, time for some OWNERs to review.

Andrew: webrtc/base/
Tommi: talk/

(Tommi, feel free to suggest someone else to review talk/ on the off chance that
you don't have a ton of free time.)

[kwiberg-webrtc, 2015-10-29 12:44:32]

Description was changed from

==========
Replace rtc::cricket::Settable with rtc::Maybe

The former is very similar to the latter, but less general (mostly in
naming).
==========

to

==========
Replace rtc::cricket::Settable with rtc::Maybe

The former is very similar to the latter, but less general (mostly in
naming).

This CL, which is the first to use Maybe at scale, also removes the implicit
conversion from T to Maybe<T>, since it was agreed that the increased verbosity
increased legibility.
==========

[tommi, 2015-10-29 16:24:19]

lgtm

[Andrew MacDonald, 2015-10-29 19:11:15]

+aluebs FYI

I'm a little sad about the loss of implicit conversions. I think there's value
in being semantically equivalent to std::experimental::optional. It will also
add unnecessary verbosity to functions with a Maybe<T> return type, e.g.:
https://codereview.webrtc.org/1394103003/diff/240001/webrtc/modules/audio_pro...

("return normal_direction" will have to become "return
rtc::Maybe<Point>(normal_direction)" right? This doesn't increase readability
because the type is clear from the function prototype.)

However, I understand the readability argument for the cases solenberg pointed
out. As long as we're not causing inconsistency with experimental::optional, I
think this change is OK, but would be interested in your opinion on the above.

lgtm

[tommi, 2015-10-29 19:21:26]

On 2015/10/29 19:11:15, Andrew MacDonald wrote:
> +aluebs FYI
> 
> I'm a little sad about the loss of implicit conversions. I think there's value
> in being semantically equivalent to std::experimental::optional. It will also
> add unnecessary verbosity to functions with a Maybe<T> return type, e.g.:
>
https://codereview.webrtc.org/1394103003/diff/240001/webrtc/modules/audio_pro...
> 
> ("return normal_direction" will have to become "return
> rtc::Maybe<Point>(normal_direction)" right? This doesn't increase readability
> because the type is clear from the function prototype.)
> 
> However, I understand the readability argument for the cases solenberg pointed
> out. As long as we're not causing inconsistency with experimental::optional, I
> think this change is OK, but would be interested in your opinion on the above.
> 
> lgtm

Agree that compatibility would be good.  I'm fine with addressing that later.

[kwiberg-webrtc, 2015-10-29 19:49:10]

On 2015/10/29 19:11:15, Andrew MacDonald wrote:

> I'm a little sad about the loss of implicit conversions. I think there's value
> in being semantically equivalent to std::experimental::optional. It will also
> add unnecessary verbosity to functions with a Maybe<T> return type, e.g.:
>
https://codereview.webrtc.org/1394103003/diff/240001/webrtc/modules/audio_pro...
> 
> ("return normal_direction" will have to become "return
> rtc::Maybe<Point>(normal_direction)" right?

Yes.

> This doesn't increase readability
> because the type is clear from the function prototype.)

Not much, no. But the language doesn't let us allow implicit conversion in one
place but not the other.

We could introduce a MakeMaybe function, so that you can at least omit the type:
"return rtc::MakeMaybe(normal_direction)". How much would that help, do you
think?

> However, I understand the readability argument for the cases solenberg pointed
> out. As long as we're not causing inconsistency with experimental::optional, I
> think this change is OK, but would be interested in your opinion on the above.

Yes, it's a tough choice. The reason I ended up agreeing with Fredrik is that
the explicit version does make the code easier to follow, and it's still
compatible with std::experimental::optional since it's a subset. Also, removing
functionality from a class like this one is going to be a pain once it's in
common use, while adding stuff is relatively painless. So we can start out using
the explicit constructions everywhere, and allow the implicit conversions later
if people want them.

[the sun, 2015-10-29 19:51:25]

On 2015/10/29 19:21:26, tommi (webrtc) wrote:
> On 2015/10/29 19:11:15, Andrew MacDonald wrote:
> > +aluebs FYI
> > 
> > I'm a little sad about the loss of implicit conversions. I think there's
value
> > in being semantically equivalent to std::experimental::optional. It will
also
> > add unnecessary verbosity to functions with a Maybe<T> return type, e.g.:
> >
>
https://codereview.webrtc.org/1394103003/diff/240001/webrtc/modules/audio_pro...
> > 
> > ("return normal_direction" will have to become "return
> > rtc::Maybe<Point>(normal_direction)" right? This doesn't increase
readability
> > because the type is clear from the function prototype.)
> > 
> > However, I understand the readability argument for the cases solenberg
pointed
> > out. As long as we're not causing inconsistency with experimental::optional,
I
> > think this change is OK, but would be interested in your opinion on the
above.
> > 
> > lgtm
> 
> Agree that compatibility would be good.  I'm fine with addressing that later.

Note that the interface is still compatible with std::experimental::optional;
requiring explicit conversions just makes it more readable.

Operator overloading is kind of write-only. It's not nearly as fun trying to
understand code that use it...

[kwiberg-webrtc, 2015-10-29 19:53:25]

On 2015/10/29 19:21:26, tommi (webrtc) wrote:
> On 2015/10/29 19:11:15, Andrew MacDonald wrote:
> > +aluebs FYI
> > 
> > I'm a little sad about the loss of implicit conversions. I think there's
value
> > in being semantically equivalent to std::experimental::optional. It will
also
> > add unnecessary verbosity to functions with a Maybe<T> return type, e.g.:
> >
>
https://codereview.webrtc.org/1394103003/diff/240001/webrtc/modules/audio_pro...
> > 
> > ("return normal_direction" will have to become "return
> > rtc::Maybe<Point>(normal_direction)" right? This doesn't increase
readability
> > because the type is clear from the function prototype.)
> > 
> > However, I understand the readability argument for the cases solenberg
pointed
> > out. As long as we're not causing inconsistency with experimental::optional,
I
> > think this change is OK, but would be interested in your opinion on the
above.
> > 
> > lgtm
> 
> Agree that compatibility would be good.  I'm fine with addressing that later.

Well, Maybe is (especially after this goes in) essentially a subset of
std::experimental::optional. So it should be painless to make the switch, or to
gradually bring it closer to std::experimental::optional. (Note the similarities
with scoped_ptr in that regard.)

[tommi, 2015-10-29 22:01:23]

On 2015/10/29 19:53:25, kwiberg-webrtc wrote:
> On 2015/10/29 19:21:26, tommi (webrtc) wrote:
> > On 2015/10/29 19:11:15, Andrew MacDonald wrote:
> > > +aluebs FYI
> > > 
> > > I'm a little sad about the loss of implicit conversions. I think there's
> value
> > > in being semantically equivalent to std::experimental::optional. It will
> also
> > > add unnecessary verbosity to functions with a Maybe<T> return type, e.g.:
> > >
> >
>
https://codereview.webrtc.org/1394103003/diff/240001/webrtc/modules/audio_pro...
> > > 
> > > ("return normal_direction" will have to become "return
> > > rtc::Maybe<Point>(normal_direction)" right? This doesn't increase
> readability
> > > because the type is clear from the function prototype.)
> > > 
> > > However, I understand the readability argument for the cases solenberg
> pointed
> > > out. As long as we're not causing inconsistency with
experimental::optional,
> I
> > > think this change is OK, but would be interested in your opinion on the
> above.
> > > 
> > > lgtm
> > 
> > Agree that compatibility would be good.  I'm fine with addressing that
later.
> 
> Well, Maybe is (especially after this goes in) essentially a subset of
> std::experimental::optional. So it should be painless to make the switch, or
to
> gradually bring it closer to std::experimental::optional. (Note the
similarities
> with scoped_ptr in that regard.)

agreed, well, lgtm stands and I agree it's easier to relax the restrictions than
tighten.

[Andrew MacDonald, 2015-10-29 22:04:50]

Still lgtm.

On 2015/10/29 19:49:10, kwiberg-webrtc wrote:
> On 2015/10/29 19:11:15, Andrew MacDonald wrote:
> > This doesn't increase readability
> > because the type is clear from the function prototype.)
> 
> Not much, no. But the language doesn't let us allow implicit conversion in one
> place but not the other.
> 
> We could introduce a MakeMaybe function, so that you can at least omit the
type:
> "return rtc::MakeMaybe(normal_direction)". How much would that help, do you
> think?

I think that's helpful enough that it'd be worth adding.

> 
> > However, I understand the readability argument for the cases solenberg
pointed
> > out. As long as we're not causing inconsistency with experimental::optional,
I
> > think this change is OK, but would be interested in your opinion on the
above.
> 
> Yes, it's a tough choice. The reason I ended up agreeing with Fredrik is that
> the explicit version does make the code easier to follow, and it's still
> compatible with std::experimental::optional since it's a subset. Also,
removing
> functionality from a class like this one is going to be a pain once it's in
> common use, while adding stuff is relatively painless. So we can start out
using
> the explicit constructions everywhere, and allow the implicit conversions
later
> if people want them.

Good point about it being easier to add features than remove them.

[kwiberg-webrtc, 2015-10-30 08:56:33]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2015-10-30 08:56:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, tommi@webrtc.org, andrew@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1430433004/#ps100001 (title:
"rebase")

[commit-bot: I haz the power, 2015-10-30 08:56:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1430433004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1430433004/100001

[commit-bot: I haz the power, 2015-10-30 09:47:44]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-10-30 09:47:54]

Patchset 5 (id:??) landed as
https://crrev.com/102c6a61bc0b42dc0956d013530fc0213b7e881b
Cr-Commit-Position: refs/heads/master@{#10461}