Disabling conversion with contained value for Optional

Summary: Comparisons with values can lead to confusion, especially when the value itself is truthy. To avoid this confusion, I'm disabling comparison with contained value. Note that Optionals can still be constructed and assigned these values, but comparsion must be done separately for the container and the contained.

Test Plan: Unit tests, contbuild

Reviewed By: tudorb@fb.com

FB internal diff: D783621

diff --git a/folly/Optional.h b/folly/Optional.h
index 871357ceabb3168b29b3da49c13886482d1d11dd..4295e271532f5466ab7cc1c992a073823fa38b2a 100644 (file)
--- a/folly/Optional.h
+++ b/folly/Optional.h
@@ -61,6 +61,7 @@
 
 #include <boost/operators.hpp>
 
+
 namespace folly {
 
 namespace detail { struct NoneHelper {}; }
@@ -81,10 +82,7 @@ const None none = nullptr;
 #endif // __GNUC__
 
 template<class Value>
-class Optional : boost::totally_ordered<Optional<Value>,
-                 boost::totally_ordered<Optional<Value>, Value>> {
-  typedef void (Optional::*bool_type)() const;
-  void truthy() const {};
+class Optional {
  public:
   static_assert(!std::is_reference<Value>::value,
                 "Optional may not be used with reference types");
@@ -110,7 +108,7 @@ class Optional : boost::totally_ordered<Optional<Value>,
     }
   }
 
-  /* implicit */ Optional(const None& empty)
+  /* implicit */ Optional(const None&)
     : hasValue_(false) {
   }
 
@@ -179,32 +177,6 @@ class Optional : boost::totally_ordered<Optional<Value>,
     return *this;
   }
 
-  bool operator<(const Optional& other) const {
-    if (hasValue() != other.hasValue()) {
-      return hasValue() < other.hasValue();
-    }
-    if (hasValue()) {
-      return value() < other.value();
-    }
-    return false; // both empty
-  }
-
-  bool operator<(const Value& other) const {
-    return !hasValue() || value() < other;
-  }
-
-  bool operator==(const Optional& other) const {
-    if (hasValue()) {
-      return other.hasValue() && value() == other.value();
-    } else {
-      return !other.hasValue();
-    }
-  }
-
-  bool operator==(const Value& other) const {
-    return hasValue() && value() == other;
-  }
-
   template<class... Args>
   void emplace(Args&&... args) {
     clear();
@@ -230,9 +202,8 @@ class Optional : boost::totally_ordered<Optional<Value>,
 
   bool hasValue() const { return hasValue_; }
 
-  /* safe bool idiom */
-  operator bool_type() const {
-    return hasValue() ? &Optional::truthy : nullptr;
+  explicit operator bool() const {
+    return hasValue();
   }
 
   const Value& operator*() const { return value(); }
@@ -286,6 +257,54 @@ Opt make_optional(T&& v) {
   return Opt(std::forward<T>(v));
 }
 
+template<class V>
+bool operator< (const Optional<V>& a, const Optional<V>& b) {
+  if (a.hasValue() != b.hasValue()) { return a.hasValue() < b.hasValue(); }
+  if (a.hasValue())                 { return a.value()    < b.value(); }
+  return false;
+}
+
+template<class V>
+bool operator==(const Optional<V>& a, const Optional<V>& b) {
+  if (a.hasValue() != b.hasValue()) { return false; }
+  if (a.hasValue())                 { return a.value() == b.value(); }
+  return true;
+}
+
+template<class V>
+bool operator<=(const Optional<V>& a, const Optional<V>& b) {
+  return !(b < a);
+}
+
+template<class V>
+bool operator!=(const Optional<V>& a, const Optional<V>& b) {
+  return !(b == a);
+}
+
+template<class V>
+bool operator>=(const Optional<V>& a, const Optional<V>& b) {
+  return !(a < b);
+}
+
+template<class V>
+bool operator> (const Optional<V>& a, const Optional<V>& b) {
+  return b < a;
+}
+
+// To supress comparability of Optional<T> with T, despite implicit conversion.
+template<class V> bool operator< (const Optional<V>&, const V& other) = delete;
+template<class V> bool operator<=(const Optional<V>&, const V& other) = delete;
+template<class V> bool operator==(const Optional<V>&, const V& other) = delete;
+template<class V> bool operator!=(const Optional<V>&, const V& other) = delete;
+template<class V> bool operator>=(const Optional<V>&, const V& other) = delete;
+template<class V> bool operator> (const Optional<V>&, const V& other) = delete;
+template<class V> bool operator< (const V& other, const Optional<V>&) = delete;
+template<class V> bool operator<=(const V& other, const Optional<V>&) = delete;
+template<class V> bool operator==(const V& other, const Optional<V>&) = delete;
+template<class V> bool operator!=(const V& other, const Optional<V>&) = delete;
+template<class V> bool operator>=(const V& other, const Optional<V>&) = delete;
+template<class V> bool operator> (const V& other, const Optional<V>&) = delete;
+
 } // namespace folly
 
 #endif//FOLLY_OPTIONAL_H_

diff --git a/folly/test/OptionalTest.cpp b/folly/test/OptionalTest.cpp
index 68e81649032a71d4a8797a364da13af01c56218c..0e035fda9f52601d852801083db4a3e6716d5d11 100644 (file)
--- a/folly/test/OptionalTest.cpp
+++ b/folly/test/OptionalTest.cpp
@@ -26,10 +26,21 @@
 #include <gtest/gtest.h>
 #include <boost/optional.hpp>
 
-using namespace folly;
 using std::unique_ptr;
 using std::shared_ptr;
 
+namespace folly {
+
+template<class V>
+std::ostream& operator<<(std::ostream& os, const Optional<V>& v) {
+  if (v) {
+    os << "Optional(" << v.value() << ')';
+  } else {
+    os << "None";
+  }
+  return os;
+}
+
 struct NoDefault {
   NoDefault(int, int) {}
   char a, b, c;
@@ -47,7 +58,7 @@ TEST(Optional, NoDefault) {
   Optional<NoDefault> x;
   EXPECT_FALSE(x);
   x.emplace(4, 5);
-  EXPECT_TRUE(x);
+  EXPECT_TRUE(bool(x));
   x.clear();
   EXPECT_FALSE(x);
 }
@@ -56,62 +67,62 @@ TEST(Optional, String) {
   Optional<std::string> maybeString;
   EXPECT_FALSE(maybeString);
   maybeString = "hello";
-  EXPECT_TRUE(maybeString);
+  EXPECT_TRUE(bool(maybeString));
 }
 
 TEST(Optional, Const) {
   { // default construct
     Optional<const int> opt;
-    EXPECT_FALSE(opt);
+    EXPECT_FALSE(bool(opt));
     opt.emplace(4);
-    EXPECT_EQ(opt, 4);
+    EXPECT_EQ(*opt, 4);
     opt.emplace(5);
-    EXPECT_EQ(opt, 5);
+    EXPECT_EQ(*opt, 5);
     opt.clear();
-    EXPECT_FALSE(opt);
+    EXPECT_FALSE(bool(opt));
   }
   { // copy-constructed
     const int x = 6;
     Optional<const int> opt(x);
-    EXPECT_EQ(opt, 6);
+    EXPECT_EQ(*opt, 6);
   }
   { // move-constructed
     const int x = 7;
     Optional<const int> opt(std::move(x));
-    EXPECT_EQ(opt, 7);
+    EXPECT_EQ(*opt, 7);
   }
   // no assignment allowed
 }
 
 TEST(Optional, Simple) {
   Optional<int> opt;
-  EXPECT_FALSE(opt);
+  EXPECT_FALSE(bool(opt));
   opt = 4;
-  EXPECT_TRUE(opt);
+  EXPECT_TRUE(bool(opt));
   EXPECT_EQ(4, *opt);
   opt = 5;
   EXPECT_EQ(5, *opt);
   opt.clear();
-  EXPECT_FALSE(opt);
+  EXPECT_FALSE(bool(opt));
 }
 
 TEST(Optional, EmptyConstruct) {
   Optional<int> opt;
-  EXPECT_FALSE(opt);
+  EXPECT_FALSE(bool(opt));
   Optional<int> test1(opt);
-  EXPECT_FALSE(test1);
+  EXPECT_FALSE(bool(test1));
   Optional<int> test2(std::move(opt));
-  EXPECT_FALSE(test2);
+  EXPECT_FALSE(bool(test2));
 }
 
 TEST(Optional, Unique) {
   Optional<unique_ptr<int>> opt;
 
   opt.clear();
-  EXPECT_FALSE(opt);
+  EXPECT_FALSE(bool(opt));
   // empty->emplaced
   opt.emplace(new int(5));
-  EXPECT_TRUE(opt);
+  EXPECT_TRUE(bool(opt));
   EXPECT_EQ(5, **opt);
 
   opt.clear();
@@ -124,24 +135,24 @@ TEST(Optional, Unique) {
 
   // move it out by move construct
   Optional<unique_ptr<int>> moved(std::move(opt));
-  EXPECT_TRUE(moved);
-  EXPECT_FALSE(opt);
+  EXPECT_TRUE(bool(moved));
+  EXPECT_FALSE(bool(opt));
   EXPECT_EQ(7, **moved);
 
-  EXPECT_TRUE(moved);
+  EXPECT_TRUE(bool(moved));
   opt = std::move(moved); // move it back by move assign
-  EXPECT_FALSE(moved);
-  EXPECT_TRUE(opt);
+  EXPECT_FALSE(bool(moved));
+  EXPECT_TRUE(bool(opt));
   EXPECT_EQ(7, **opt);
 }
 
 TEST(Optional, Shared) {
   shared_ptr<int> ptr;
   Optional<shared_ptr<int>> opt;
-  EXPECT_FALSE(opt);
+  EXPECT_FALSE(bool(opt));
   // empty->emplaced
   opt.emplace(new int(5));
-  EXPECT_TRUE(opt);
+  EXPECT_TRUE(bool(opt));
   ptr = opt.value();
   EXPECT_EQ(ptr.get(), opt->get());
   EXPECT_EQ(2, ptr.use_count());
@@ -185,7 +196,7 @@ TEST(Optional, Order) {
     { 3 },
   };
   std::sort(vect.begin(), vect.end());
-  EXPECT_TRUE(vect == expected);
+  EXPECT_EQ(vect, expected);
 }
 
 TEST(Optional, Swap) {
@@ -218,14 +229,9 @@ TEST(Optional, Comparisons) {
   Optional<int> o1(1);
   Optional<int> o2(2);
 
-  EXPECT_TRUE(o_ < 1);
-  EXPECT_TRUE(o_ <= 1);
   EXPECT_TRUE(o_ <= o_);
   EXPECT_TRUE(o_ == o_);
-  EXPECT_TRUE(o_ != 1);
   EXPECT_TRUE(o_ >= o_);
-  EXPECT_TRUE(1 >= o_);
-  EXPECT_TRUE(1 > o_);
 
   EXPECT_TRUE(o1 < o2);
   EXPECT_TRUE(o1 <= o2);
@@ -245,6 +251,7 @@ TEST(Optional, Comparisons) {
   EXPECT_FALSE(o1 >= o2);
   EXPECT_FALSE(o1 > o2);
 
+  /* folly::Optional explicitly doesn't support comparisons with contained value
   EXPECT_TRUE(1 < o2);
   EXPECT_TRUE(1 <= o2);
   EXPECT_TRUE(1 <= o1);
@@ -262,6 +269,68 @@ TEST(Optional, Comparisons) {
   EXPECT_FALSE(o1 >= 2);
   EXPECT_FALSE(o1 >= 2);
   EXPECT_FALSE(o1 > 2);
+  */
+
+  // boost::optional does support comparison with contained value, which can
+  // lead to confusion when a bool is contained
+  boost::optional<int> boi(3);
+  EXPECT_TRUE(boi < 5);
+  EXPECT_TRUE(boi <= 4);
+  EXPECT_TRUE(boi == 3);
+  EXPECT_TRUE(boi != 2);
+  EXPECT_TRUE(boi >= 1);
+  EXPECT_TRUE(boi > 0);
+  EXPECT_TRUE(1 <  boi);
+  EXPECT_TRUE(2 <= boi);
+  EXPECT_TRUE(3 == boi);
+  EXPECT_TRUE(4 != boi);
+  EXPECT_TRUE(5 >= boi);
+  EXPECT_TRUE(6 >  boi);
+
+  boost::optional<bool> bob(false);
+  EXPECT_TRUE(bob);
+  EXPECT_TRUE(bob == false); // well that was confusing
+  EXPECT_FALSE(bob != false);
+}
+
+TEST(Optional, Conversions) {
+  Optional<bool> mbool;
+  Optional<short> mshort;
+  Optional<char*> mstr;
+  Optional<int> mint;
+
+  //These don't compile
+  //bool b = mbool;
+  //short s = mshort;
+  //char* c = mstr;
+  //int x = mint;
+  //char* c(mstr);
+  //short s(mshort);
+  //int x(mint);
+
+  // intended explicit operator bool, for if (opt).
+  bool b(mbool);
+
+  // Truthy tests work and are not ambiguous
+  if (mbool && mshort && mstr && mint) { // only checks not-empty
+    if (*mbool && *mshort && *mstr && *mint) { // only checks value
+      ;
+    }
+  }
+
+  mbool = false;
+  EXPECT_TRUE(bool(mbool));
+  EXPECT_FALSE(*mbool);
+
+  mbool = true;
+  EXPECT_TRUE(bool(mbool));
+  EXPECT_TRUE(*mbool);
+
+  mbool = none;
+  EXPECT_FALSE(bool(mbool));
+
+  // No conversion allowed; does not compile
+  // EXPECT_TRUE(mbool == false);
 }
 
 TEST(Optional, Pointee) {
@@ -270,7 +339,7 @@ TEST(Optional, Pointee) {
   x = 1;
   EXPECT_TRUE(get_pointer(x));
   *get_pointer(x) = 2;
-  EXPECT_TRUE(x == 2);
+  EXPECT_TRUE(*x == 2);
   x = none;
   EXPECT_FALSE(get_pointer(x));
 }
@@ -353,3 +422,5 @@ TEST(Optional, AssignmentContained) {
     EXPECT_FALSE(target.hasValue());
   }
 }
+
+}