Rename Maybe to Optional

webrtc/base/optional.h

Index: webrtc/base/optional.h
diff --git a/webrtc/base/maybe.h b/webrtc/base/optional.h
similarity index 54%
rename from webrtc/base/maybe.h
rename to webrtc/base/optional.h
index 1df94def3f063709a47d9fdd0244c00c1657fc14..6e7535b44600a3713f13e95993b7516de6040e44 100644
--- a/webrtc/base/maybe.h
+++ b/webrtc/base/optional.h
@@ -8,8 +8,8 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
-#ifndef WEBRTC_BASE_MAYBE_H_
-#define WEBRTC_BASE_MAYBE_H_
+#ifndef WEBRTC_BASE_OPTIONAL_H_
+#define WEBRTC_BASE_OPTIONAL_H_
 
 #include <algorithm>
 #include <utility>
@@ -24,38 +24,67 @@ namespace rtc {
 // contain a value; use e.g. rtc::scoped_ptr<T> instead if that's too
 // expensive.
 //
-// A moved-from Maybe<T> may only be destroyed, and assigned to if T allows
+// A moved-from Optional<T> may only be destroyed, and assigned to if T allows
 // being assigned to after having been moved from. Specifically, you may not
 // assume that it just doesn't contain a value anymore.
 //
+// Examples of good places to use Optional:
+//
+// - As a class or struct member, when the member doesn't always have a value:
+//     struct Prisoner {
+//       std::string name;
+//       Optional<int> cell_number;  // Empty if not currently incarcerated.
+//     };
+//
+// - As a return value for functions that may fail to return a value on all
+//   allowed inputs. For example, a function that searches an array might
+//   return an Optional<size_t> (the index where it found the element, or
+//   nothing if it didn't find it); and a function that parses numbers might
+//   return Optional<double> (the parsed number, or nothing if parsing failed).
+//
+// Examples of bad places to use Optional:
+//
+// - As a return value for functions that may fail because of disallowed
+//   inputs. For example, a string length function should not return
+//   Optional<size_t> so that it can return nothing in case the caller passed
+//   it a null pointer; the function should probably use RTC_[D]CHECK instead,
+//   and return plain size_t.
+//
+// - As a return value for functions that may fail to return a value on all
+//   allowed inputs, but need to tell the caller what went wrong. Returning
+//   Optional<double> when parsing a single number as in the example above
+//   might make sense, but any larger parse job is probably going to need to
+//   tell the caller what the problem was, not just that there was one.
+//
 // TODO(kwiberg): Get rid of this class when the standard library has
 // std::optional (and we're allowed to use it).
 template <typename T>
-class Maybe final {
+class Optional final {
  public:
-  // Construct an empty Maybe.
-  Maybe() : has_value_(false) {}
+  // Construct an empty Optional.
+  Optional() : has_value_(false) {}
 
-  // Construct a Maybe that contains a value.
-  explicit Maybe(const T& val) : value_(val), has_value_(true) {}
-  explicit Maybe(T&& val) : value_(static_cast<T&&>(val)), has_value_(true) {}
+  // Construct an Optional that contains a value.
+  explicit Optional(const T& val) : value_(val), has_value_(true) {}
+  explicit Optional(T&& val)
+      : value_(static_cast<T&&>(val)), has_value_(true) {}

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

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:56]

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]

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.
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.)
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]

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.
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]

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).
Yes, Optional<bool> is a bit scary that way. A named method for testing if it's
set would have been safer.
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]

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'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 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. :-)
Oh, you don't have to do that. Just

  foo = Optional<T>(5);

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

 
   // Copy and move constructors.
   // TODO(kwiberg): =default the move constructor when MSVC supports it.
-  Maybe(const Maybe&) = default;
-  Maybe(Maybe&& m)
+  Optional(const Optional&) = default;
+  Optional(Optional&& m)
       : value_(static_cast<T&&>(m.value_)), has_value_(m.has_value_) {}
 
   // Assignment.
   // TODO(kwiberg): =default the move assignment op when MSVC supports it.
-  Maybe& operator=(const Maybe&) = default;
-  Maybe& operator=(Maybe&& m) {
+  Optional& operator=(const Optional&) = default;
+  Optional& operator=(Optional&& m) {
     value_ = static_cast<T&&>(m.value_);
     has_value_ = m.has_value_;
     return *this;
   }
 
-  friend void swap(Maybe& m1, Maybe& m2) {
+  friend void swap(Optional& m1, Optional& m2) {
     using std::swap;
     swap(m1.value_, m2.value_);
     swap(m1.has_value_, m2.has_value_);
@@ -87,13 +116,14 @@ class Maybe final {
     return has_value_ ? value_ : default_val;
   }
 
-  // Equality tests. Two Maybes are equal if they contain equivalent values, or
+  // Equality tests. Two Optionals are equal if they contain equivalent values,
+  // or
   // if they're both empty.
-  friend bool operator==(const Maybe& m1, const Maybe& m2) {
+  friend bool operator==(const Optional& m1, const Optional& m2) {
     return m1.has_value_ && m2.has_value_ ? m1.value_ == m2.value_
                                           : m1.has_value_ == m2.has_value_;
   }
-  friend bool operator!=(const Maybe& m1, const Maybe& m2) {
+  friend bool operator!=(const Optional& m1, const Optional& m2) {
     return m1.has_value_ && m2.has_value_ ? m1.value_ != m2.value_
                                           : m1.has_value_ != m2.has_value_;
   }
@@ -107,4 +137,4 @@ class Maybe final {
 
 }  // namespace rtc
 
-#endif  // WEBRTC_BASE_MAYBE_H_
+#endif  // WEBRTC_BASE_OPTIONAL_H_