Base: add Optional<T>.

base/optional.h

Index: base/optional.h
diff --git a/base/optional.h b/base/optional.h
new file mode 100644
index 0000000000000000000000000000000000000000..c7eae67bf6ac7677b2600f72b6e98e6fa771b2c3
--- /dev/null
+++ b/base/optional.h
@@ -0,0 +1,421 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_OPTIONAL_H_
+#define BASE_OPTIONAL_H_
+
+#include "base/logging.h"
+#include "base/memory/aligned_memory.h"
+
+namespace base {
+
+// Specification:
+// http://en.cppreference.com/w/cpp/experimental/optional/in_place_t
+struct in_place_t {};
+
+// Specification:
+// http://en.cppreference.com/w/cpp/experimental/optional/nullopt_t
+struct nullopt_t {
+  explicit nullopt_t(int) { }

[danakj, 2016/03/04 00:31:08]

Can you throw a TODO on here that it should be constexptr once MSVC 2015?

[danakj, 2016/03/04 00:31:09]

nit: no space in {}. did you git cl format?

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Done.
Done. I didn't use `git cl format`, will do.

+};
+
+// Specification:
+// http://en.cppreference.com/w/cpp/experimental/optional/in_place
+const in_place_t in_place{};

[danakj, 2016/03/04 00:31:09]

this is brace-initialization which we don't allow yet. you can just write this
as below?

const in_place_t in_place;

Also, TODO for constexpr?

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Done.

+
+// Specification:
+// http://en.cppreference.com/w/cpp/experimental/optional/nullopt
+const nullopt_t nullopt(0);

[danakj, 2016/03/04 00:31:08]

TODO for constexpr here too?

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Done.

+
+// base::Optional is a Chromium version of the C++ library experimental optional
+// class: http://en.cppreference.com/w/cpp/experimental/optional/optional

[danakj, 2016/03/04 00:31:08]

Link should be http://en.cppreference.com/w/cpp/experimental/optional (you're
linking to constructor)

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Done.

+// The following known differences apply:
+// - 'constexpr' is not used because it is currently banned from Chromium.
+// - The constructor and emplace method using initializer_list are not
+//   implemented because 'initializer_list' is banned from Chromium.
+// - No exceptions are thrown, because they are banned from Chromium.
+// - Private member |val| (for exposition) is not implemented.

[danakj, 2016/03/04 00:31:08]

The |val| is just an example, I don't think you need to mention it here.

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Done.

+// - The destructor doesn't use std::std::is_trivially_destructible.

[danakj, 2016/03/04 00:31:08]

can you explain why? msvc problems?

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Didn't even work locally. I've added a TODO.

+// - All the non-members are in the 'base' namespace instead of `std'.`

[danakj, 2016/03/04 00:31:07]

quotes got a little weird around "std"

[mlamouri (slow - plz ping), 2016/03/18 01:16:02]

Done.

+template <typename T>
+class Optional {
+ public:
+  Optional() = default;
+  Optional(base::nullopt_t opt) : Optional() { }

[danakj, 2016/03/04 00:31:08]

nit: you could leave the parameter unnamed

[mlamouri (slow - plz ping), 2016/03/18 01:16:04]

Done.

+
+  Optional(const Optional& other) {
+    if (!other.is_null_)
+      init(other.value());
+  }
+
+  Optional(Optional&& other) {
+    if (!other.is_null_)
+      init(std::move(*other.buffer_.template data_as<T>()));

[danakj, 2016/03/04 00:31:08]

why not std::move(other.value()) ? it should cast the T& to a T&& to do what you
want?

[mlamouri (slow - plz ping), 2016/03/18 01:16:02]

Hmm, I think I've implemented this method before .value() returning T&&.

Done.

+  }
+
+  Optional(const T& value) {
+    init(value);
+  }
+
+  Optional(T&& value) {
+    init(std::move(value));
+  }
+
+  template <class... Args>
+  explicit Optional(base::in_place_t, Args&&... args) {
+    emplace(std::forward<Args>(args)...);
+  }
+
+  ~Optional() {
+    free_if_needed();
+  }
+
+  Optional& operator=(base::nullopt_t opt) {

[danakj, 2016/03/04 00:31:08]

nit: can leave the parameter unnamed

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Done.

+    free_if_needed();
+    return *this;
+  }
+
+  Optional& operator=(const Optional& other) {
+    if (other.is_null_) {
+      free_if_needed();
+      return *this;
+    }
+
+    init_or_assign(other.value());
+    return *this;
+  }
+
+  Optional& operator=(Optional&& other) {
+    if (other.is_null_) {
+      free_if_needed();
+      return *this;
+    }
+
+    init_or_assign(std::move(*other.buffer_.template data_as<T>()));

[danakj, 2016/03/04 00:31:09]

why not std::move(other.value()) ? it should cast the T& to a T&& to do what you
want?

[mlamouri (slow - plz ping), 2016/03/18 01:16:04]

As above.

Done.

+    return *this;
+  }
+
+  template<class U>
+  auto operator=(U&& value)
+  -> typename std::enable_if
+<std::is_same<std::decay<U>, T>::value, Optional&>::type

[danakj, 2016/03/04 00:31:07]

The formatting here.. is that clang-format?

Can you write this with the enable_if on the left side of the function name
(instead of using auto -> syntax)?

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

No. This CL wasn't meant to be done but just for you to have a look. Will run
`git cl format`.
Done.

+  {
+    init_or_assign(std::forward<U>(value));
+    return *this;
+  }
+
+  const T* operator->() const {
+    DCHECK(!is_null_);
+    return buffer_.template data_as<T>();

[danakj, 2016/03/04 00:31:07]

return &value()?

[mlamouri (slow - plz ping), 2016/03/18 01:16:04]

Done.

+  }
+
+  T* operator->() {
+    DCHECK(!is_null_);
+    return buffer_.template data_as<T>();

[danakj, 2016/03/04 00:31:07]

return &value()?

[mlamouri (slow - plz ping), 2016/03/18 01:16:04]

Done.

+  }
+
+  const T& operator*() const & {
+    return value();
+  }
+
+  T& operator*() & {
+    return value();
+  }
+
+  const T&& operator*() const && {
+    return std::forward<T>(value());

[danakj, 2016/03/04 00:31:08]

why forward? just std::move(value())?

optional won't compile if T is a reference type for many reasons.

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Done.

+  }
+
+  T&& operator*() && {
+    return std::forward<T>(value());

[danakj, 2016/03/04 00:31:09]

std::move(value())?

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Done.

+  }
+
+#if 1
+  explicit operator bool() const { return !is_null_; }
+#else
+  // Allow Optional<T> to be used in boolean expressions, but not implicitely
+  // convertible to a real bool (which is dangerous). Nor using an explicit bool
+  // operator (which is not allowed).
+  //
+  // Note that this trick is only safe when the == and != operators are declared
+  // explicitly, as otherwise "optional_1 == optional_2" will compile but do the
+  // wrong thing (i.e., convert to Testable and then do the comparison).
+  //
+  // Note that this isn't following the C++ specification which requires using
+  // an explicit bool operator.
+ private:
+  typedef T* Optional::*Testable;

[danakj, 2016/03/04 00:31:08]

using Optional::*Testable = bool?

[danakj, 2016/03/04 00:31:09]

prefer "using" to "typedef"

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Done (deprecated).

+
+ public:
+  // TODO: make this work.
+  operator Testable() const {
+    return is_null_ ? nullptr : &Optional::buffer_.template data_as<T>();

[danakj, 2016/03/04 00:31:08]

return is_null_ ? nullptr : &Optional::is_null_ ?


So.. I have good news and bad news.

Bad news: Since this class uses ref qualifiers, it won't compile in MSVC 2013.
So we'll have to wait for 2015.
Good news: 2015 continues to be very soon, I heard say this week again? Next
week? idk
Better news: That means you can just use explicit operator, cuz that will work
in MSVC 2015 too.

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

\o/

I removed the dirty workaround.

+  }
+#endif
+
+  T& value() & {
+    DCHECK(!is_null_);
+    return *buffer_.template data_as<T>();
+  }
+
+  const T& value() const & {
+    DCHECK(!is_null_);
+    return *buffer_.template data_as<T>();
+  }
+
+  T&& value() && {
+    DCHECK(!is_null_);
+    return std::move(*buffer_.template data_as<T>());
+  }
+
+  const T&& value() const && {
+    DCHECK(!is_null_);
+    return std::move(*buffer_.template data_as<T>());
+  }
+
+  template <class U>
+  T value_or(U&& default_value) const {

[danakj, 2016/03/04 00:31:08]

this should be qualified as const&

[danakj, 2016/03/04 00:31:08]

can you add some static asserts here:

// TODO(danakj): Enable this when MSVC 2015 happens.
// static_assert(std::is_copy_constructible<T>::value, "T must be copy
constructible");
static_assert(std::is_convertible<U, T>::value, "U must be convertible to T");

i'm assuming the 2nd one would work now..

[mlamouri (slow - plz ping), 2016/03/18 01:16:04]

Done.

+    return is_null_ ? default_value : value();

[danakj, 2016/03/04 00:31:07]

i think in the null case you need to actually return

static_cast<T>(std::forward<U>(default_value))

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Oups. I completely missed that in the spec.

Done.

+  }
+
+  template <class U>
+  T value_or(U&& default_value) {

[danakj, 2016/03/04 00:31:08]

can you add some static asserts here:

// TODO(danakj): Enable this when MSVC 2015 happens.
// static_assert(std::is_move_constructible<T>::value, "T must be move
constructible");
static_assert(std::is_convertible<U, T>::value, "U must be convertible to T");

i'm assuming the 2nd one would work now..

[danakj, 2016/03/04 00:31:08]

this should be qualified as &&

[mlamouri (slow - plz ping), 2016/03/18 01:16:04]

Done.
Done.

+    return is_null_ ? default_value : value();

[danakj, 2016/03/04 00:31:08]

in null case: return static_cast<T>(std::forward<U>(default_value))
in non-null case: return std::move(value())

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Thanks for noticing. I missed this from the spec.

Done.

+  }
+
+  void swap(Optional& other) {
+    if (is_null_ && other.is_null_)
+      return;
+
+    if (is_null_ != other.is_null_) {
+      if (is_null_) {
+        init(std::move(*other.buffer_.template data_as<T>()));
+        other.free_if_needed();
+      } else {
+        other.init(std::move(*buffer_.template data_as<T>()));
+        free_if_needed();
+      }
+      return;
+    }
+
+    DCHECK(!is_null_ && !other.is_null_);
+    using std::swap;
+    swap(**this, *other);

[danakj, 2016/03/04 00:31:07]

I think this has to be a typo in cppreference? It should be swap(value(),
other.value())? The implementation in libcxx seems to agree.

Is there a unit test case for this?

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Yes: OptionalTest.Swap_*

+  }
+
+  template <class... Args>
+  void emplace(Args&&... args) {
+    free_if_needed();
+    init(T(std::forward<Args>(args)...));

[danakj, 2016/03/04 00:31:09]

This isn't quite the same thing as constructing from the arguments, this is
constructing and then moving. You want to forward the args to init() (you'll
need another init for this or skip using init() here)

[mlamouri (slow - plz ping), 2016/03/18 01:16:04]

Done... I think :)

+  }
+
+ private:
+  void init(const T& value) {

[danakj, 2016/03/04 00:31:08]

can you name these private methods in CamelCase style? They aren't necessarily
that cheap.

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Done. However, my intention was to keep a consistent style with-in the file.

+    DCHECK(is_null_);
+    new (buffer_.template data_as<T>()) T(value);
+    is_null_ = false;
+  }
+
+  void init(T&& value) {
+    DCHECK(is_null_);
+    new (buffer_.template data_as<T>()) T(std::forward<T>(value));

[danakj, 2016/03/04 00:31:08]

just move() here, not forward. the function is not separately templated, so it
will always be an rvalue ref.

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Done.

+    is_null_ = false;
+  }
+
+  void init_or_assign(const T& value) {
+    if (is_null_)
+      init(value);
+    else
+      *buffer_.template data_as<T>() = value;
+  }
+
+  void init_or_assign(T&& value) {
+    if (is_null_)
+      init(std::forward<T>(value));

[danakj, 2016/03/04 00:31:09]

just std::move() here

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Done.

+    else
+      *buffer_.template data_as<T>() = std::forward<T>(value);

[danakj, 2016/03/04 00:31:07]

and here

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

Done.

+  }
+
+  void free_if_needed() {
+    if (is_null_)
+      return;
+    buffer_.template data_as<T>()->~T();
+    is_null_ = true;
+  }
+
+  bool is_null_ = true;
+  base::AlignedMemory<sizeof(T), ALIGNOF(T)> buffer_;
+};
+
+template <class T>
+bool operator==(const Optional<T>& lhs, const Optional<T>& rhs) {
+  if (bool(lhs) != bool(rhs))

[danakj, 2016/03/04 00:31:08]

These are cast-styles that the google style guide doesn't like. Instead you can
write static_cast<bool>(lhs) or !!lhs or lhs != nullopt.

using nullopt is the most verbose but may also be the most clear, especially in
cases where you are negating the bool cast.

[mlamouri (slow - plz ping), 2016/03/18 01:16:04]

Done.

+    return false;
+  return !bool(lhs) || (*lhs == *rhs);
+}
+
+template <class T>
+bool operator!=(const Optional<T>& lhs, const Optional<T>& rhs) {
+  return !(lhs == rhs);
+}
+
+template <class T>
+bool operator<(const Optional<T>& lhs, const Optional<T>& rhs) {
+  if (!bool(rhs))
+    return false;
+  if (!bool(lhs))
+    return true;
+  return *lhs < *rhs;
+}
+
+template <class T>
+bool operator<=(const Optional<T>& lhs, const Optional<T>& rhs) {
+  return !(rhs < lhs);
+}
+
+template <class T>
+bool operator>(const Optional<T>& lhs, const Optional<T>& rhs) {
+  return rhs < lhs;
+}
+
+template <class T>
+bool operator>=(const Optional<T>& lhs, const Optional<T>& rhs) {
+  return !(lhs < rhs);
+}
+
+template <class T>
+bool operator==(const Optional<T>& opt, base::nullopt_t) {
+  return !opt;
+}
+
+template <class T>
+bool operator==(base::nullopt_t, const Optional<T>& opt) {
+  return !opt;
+}
+
+template <class T>
+bool operator!=(const Optional<T>& opt, base::nullopt_t) {
+  return bool(opt);
+}
+
+template <class T>
+bool operator!=(base::nullopt_t, const Optional<T>& opt) {
+  return bool(opt);
+}
+
+template <class T>
+bool operator<(const Optional<T>& opt, base::nullopt_t) {
+  return false;
+}
+
+template <class T>
+bool operator<(base::nullopt_t, const Optional<T>& opt) {
+  return bool(opt);
+}
+
+template <class T>
+bool operator<=(const Optional<T>& opt, base::nullopt_t) {
+  return !opt;
+}
+
+template <class T>
+bool operator<=(base::nullopt_t, const Optional<T>& opt) {
+  return true;
+}
+
+template <class T>
+bool operator>(const Optional<T>& opt, base::nullopt_t) {
+  return bool(opt);

[danakj, 2016/03/04 00:31:08]

can you write the > operators here and below in terms of the < operators above?

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

I could but my goal here was to follow the spec as much as possible and the
returned value as defined more or less as I wrote them here.

[danakj, 2016/03/21 21:49:37]

I'd still prefer that you wrote them in terms of previously written operators.
They should still match the spec behaviour.

+}
+
+template <class T>
+bool operator>(base::nullopt_t, const Optional<T>& opt) {
+  return false;
+}
+
+template <class T>
+bool operator>=(const Optional<T>& opt, base::nullopt_t) {
+  return true;
+}
+
+template <class T>
+bool operator>=(base::nullopt_t, const Optional<T>& opt) {
+  return !opt;
+}
+
+template <class T>
+bool operator==(const Optional<T>& opt, const T& value) {
+  return bool(opt) ? *opt == value : false;
+}
+
+template <class T>
+bool operator==(const T& value, const Optional<T>& opt) {
+  return bool(opt) ? value == *opt : false;
+}
+
+template <class T>
+bool operator!=(const Optional<T>& opt, const T& value) {
+  return bool(opt) ? *opt != value : true;
+}
+
+template <class T>
+bool operator!=(const T& value, const Optional<T>& opt) {
+  return bool(opt) ? value != *opt : true;
+}
+
+template <class T>
+bool operator<(const Optional<T>& opt, const T& value) {
+  return bool(opt) ? *opt < value : true;
+}
+
+template <class T>
+bool operator<(const T& value, const Optional<T>& opt) {
+  return bool(opt) ? value < *opt : false;
+}
+
+template <class T>
+bool operator<=(const Optional<T>& opt, const T& value) {
+  return !(opt > value);
+}
+
+template <class T>
+bool operator<=(const T& value, const Optional<T>& opt) {
+  return !(value > opt);
+}
+
+template <class T>
+bool operator>(const Optional<T>& opt, const T& value) {
+  return bool(opt) ? value < *opt : false;
+}
+
+template <class T>
+bool operator>(const T& value, const Optional<T>& opt) {
+  return bool(opt) ? *opt < value : true;

[danakj, 2016/03/04 00:31:08]

same here, write these in terms of the operator < and friends?

[mlamouri (slow - plz ping), 2016/03/18 01:16:03]

As above.

+}
+
+template <class T>
+bool operator>=(const Optional<T>& opt, const T& value) {
+  return !(opt < value);
+}
+
+template <class T>
+bool operator>=(const T& value, const Optional<T>& opt) {
+  return !(value < opt);
+}
+
+template <class T>
+Optional<typename std::decay<T>::type> make_optional(T&& value) {
+  return Optional<typename std::decay<T>::type>(std::forward<T>(value));
+}
+
+template <class T>
+void swap(Optional<T>& lhs, Optional<T>& rhs) {
+  lhs.swap(rhs);
+}
+
+// TODO: implement hash function.

[danakj, 2016/03/04 00:31:08]

put a name or a bug on it

[mlamouri (slow - plz ping), 2016/03/18 01:16:04]

Implemented it.

+
+}  // namespace base
+
+#endif // BASE_OPTIONAL_H_