From 7ba9e92fa0dfb16579f4f6ecd746397bdfdd174d Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Mon, 21 May 2018 09:57:36 +0200 Subject: [PATCH] Use absl::optional instead or rtc::Optional BUG: webrtc:9078 Change-Id: I69aedce324d86e8894b81210a2de17c5ef68fd11 Reviewed-on: https://webrtc-review.googlesource.com/77082 Commit-Queue: Danil Chapovalov Reviewed-by: Karl Wiberg Reviewed-by: Mirko Bonadei Cr-Commit-Position: refs/heads/master@{#23440} --- DEPS | 3 + api/BUILD.gn | 10 +- api/optional.cc | 34 ---- api/optional.h | 426 +-------------------------------------- api/optional_unittest.cc | 66 +++--- 5 files changed, 49 insertions(+), 490 deletions(-) delete mode 100644 api/optional.cc diff --git a/DEPS b/DEPS index e98c8fda64..f4053cdea0 100644 --- a/DEPS +++ b/DEPS @@ -734,4 +734,7 @@ include_rules = [ "+rtc_base", "+test", "+rtc_tools", + + # Abseil whitelist. + "+absl/types/optional.h", ] diff --git a/api/BUILD.gn b/api/BUILD.gn index bc498ac569..e693814b8e 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -230,16 +230,15 @@ rtc_source_set("array_view") { ] } +# TODO(bugs.webrtc.org/9078): Deprecated, replaced by absl/types:optional. +# Delete after webrtc and downstreams users are updated. rtc_source_set("optional") { visibility = [ "*" ] sources = [ - "optional.cc", "optional.h", ] deps = [ - ":array_view", - "../rtc_base:checks", - "../rtc_base:sanitizer", + "//third_party/abseil-cpp/absl/types:optional", ] } @@ -381,6 +380,9 @@ if (rtc_include_tests) { sources = [ "array_view_unittest.cc", + + # TODO(bugs.webrtc.org/8821): Remove optional_unittests when webrtc starts + # running absl unittest on each commit. "optional_unittest.cc", "ortc/mediadescription_unittest.cc", "ortc/sessiondescription_unittest.cc", diff --git a/api/optional.cc b/api/optional.cc deleted file mode 100644 index 0f74bd2446..0000000000 --- a/api/optional.cc +++ /dev/null @@ -1,34 +0,0 @@ -/* - * Copyright 2016 The WebRTC Project Authors. All rights reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "api/optional.h" - -namespace rtc { -namespace optional_internal { - -#if RTC_HAS_ASAN - -const void* FunctionThatDoesNothingImpl(const void* x) { - return x; -} - -#endif - -struct NulloptArg { - constexpr NulloptArg() {} -}; - -static NulloptArg nullopt_arg; - -} // namespace optional_internal - -const nullopt_t nullopt(rtc::optional_internal::nullopt_arg); - -} // namespace rtc diff --git a/api/optional.h b/api/optional.h index ba06831a29..eada13fa9b 100644 --- a/api/optional.h +++ b/api/optional.h @@ -8,435 +8,19 @@ * be found in the AUTHORS file in the root of the source tree. */ +// TODO(bugs.webrtc.org/9078): Use absl::optional directly. #ifndef API_OPTIONAL_H_ #define API_OPTIONAL_H_ -#include -#include -#include - -#ifdef UNIT_TEST -#include -#include -#endif // UNIT_TEST - -#include "api/array_view.h" -#include "rtc_base/checks.h" -#include "rtc_base/sanitizer.h" +#include "absl/types/optional.h" namespace rtc { -namespace optional_internal { - -#if RTC_HAS_ASAN - -// This is a non-inlined function. The optimizer can't see inside it. It -// prevents the compiler from generating optimized code that reads value_ even -// if it is unset. Although safe, this causes memory sanitizers to complain. -const void* FunctionThatDoesNothingImpl(const void*); +using absl::nullopt_t; +using absl::nullopt; template -inline const T* FunctionThatDoesNothing(T* x) { - return reinterpret_cast( - FunctionThatDoesNothingImpl(reinterpret_cast(x))); -} - -#else - -template -inline const T* FunctionThatDoesNothing(T* x) { - return x; -} - -#endif - -struct NulloptArg; - -} // namespace optional_internal - -// nullopt_t must be a non-aggregate literal type with a constexpr constructor -// that takes some implementation-defined literal type. It mustn't have a -// default constructor nor an initializer-list constructor. -// See: -// http://en.cppreference.com/w/cpp/utility/optional/nullopt_t -// That page uses int, though this seems to confuse older versions of GCC. -struct nullopt_t { - constexpr explicit nullopt_t(rtc::optional_internal::NulloptArg&) {} -}; - -// Specification: -// http://en.cppreference.com/w/cpp/utility/optional/nullopt -extern const nullopt_t nullopt; - -// Simple std::optional-wannabe. It either contains a T or not. -// -// A moved-from Optional 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 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 (the index where it found the element, or -// nothing if it didn't find it); and a function that parses numbers might -// return Optional (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 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 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. -// -// - As a non-mutable function argument. When you want to pass a value of a -// type T that can fail to be there, const T* is almost always both fastest -// and cleanest. (If you're *sure* that the the caller will always already -// have an Optional, const Optional& is slightly faster than const T*, -// but this is a micro-optimization. In general, stick to const T*.) -// -// TODO(kwiberg): Get rid of this class when the standard library has -// std::optional (and we're allowed to use it). -template -class Optional final { - public: - // Construct an empty Optional. - Optional() : has_value_(false), empty_('\0') { PoisonValue(); } - - Optional(rtc::nullopt_t) // NOLINT(runtime/explicit) - : Optional() {} - - // Construct an Optional that contains a value. - Optional(const T& value) // NOLINT(runtime/explicit) - : has_value_(true) { - new (&value_) T(value); - } - Optional(T&& value) // NOLINT(runtime/explicit) - : has_value_(true) { - new (&value_) T(std::move(value)); - } - - // Copy constructor: copies the value from m if it has one. - Optional(const Optional& m) : has_value_(m.has_value_) { - if (has_value_) - new (&value_) T(m.value_); - else - PoisonValue(); - } - - // Move constructor: if m has a value, moves the value from m, leaving m - // still in a state where it has a value, but a moved-from one (the - // properties of which depends on T; the only general guarantee is that we - // can destroy m). - Optional(Optional&& m) : has_value_(m.has_value_) { - if (has_value_) - new (&value_) T(std::move(m.value_)); - else - PoisonValue(); - } - - ~Optional() { - if (has_value_) - value_.~T(); - else - UnpoisonValue(); - } - - Optional& operator=(rtc::nullopt_t) { - reset(); - return *this; - } - - // Copy assignment. Uses T's copy assignment if both sides have a value, T's - // copy constructor if only the right-hand side has a value. - Optional& operator=(const Optional& m) { - if (m.has_value_) { - if (has_value_) { - value_ = m.value_; // T's copy assignment. - } else { - UnpoisonValue(); - new (&value_) T(m.value_); // T's copy constructor. - has_value_ = true; - } - } else { - reset(); - } - return *this; - } - - // Move assignment. Uses T's move assignment if both sides have a value, T's - // move constructor if only the right-hand side has a value. The state of m - // after it's been moved from is as for the move constructor. - Optional& operator=(Optional&& m) { - if (m.has_value_) { - if (has_value_) { - value_ = std::move(m.value_); // T's move assignment. - } else { - UnpoisonValue(); - new (&value_) T(std::move(m.value_)); // T's move constructor. - has_value_ = true; - } - } else { - reset(); - } - return *this; - } - - // Swap the values if both m1 and m2 have values; move the value if only one - // of them has one. - friend void swap(Optional& m1, Optional& m2) { - if (m1.has_value_) { - if (m2.has_value_) { - // Both have values: swap. - using std::swap; - swap(m1.value_, m2.value_); - } else { - // Only m1 has a value: move it to m2. - m2.UnpoisonValue(); - new (&m2.value_) T(std::move(m1.value_)); - m1.value_.~T(); // Destroy the moved-from value. - m1.has_value_ = false; - m2.has_value_ = true; - m1.PoisonValue(); - } - } else if (m2.has_value_) { - // Only m2 has a value: move it to m1. - m1.UnpoisonValue(); - new (&m1.value_) T(std::move(m2.value_)); - m2.value_.~T(); // Destroy the moved-from value. - m1.has_value_ = true; - m2.has_value_ = false; - m2.PoisonValue(); - } - } - - // Destroy any contained value. Has no effect if we have no value. - void reset() { - if (!has_value_) - return; - value_.~T(); - has_value_ = false; - PoisonValue(); - } - - template - void emplace(Args&&... args) { - if (has_value_) - value_.~T(); - else - UnpoisonValue(); - new (&value_) T(std::forward(args)...); - has_value_ = true; - } - - // Conversion to bool to test if we have a value. - explicit operator bool() const { return has_value_; } - bool has_value() const { return has_value_; } - - // Dereferencing. Only allowed if we have a value. - const T* operator->() const { - RTC_DCHECK(has_value_); - return &value_; - } - T* operator->() { - RTC_DCHECK(has_value_); - return &value_; - } - const T& operator*() const { - RTC_DCHECK(has_value_); - return value_; - } - T& operator*() { - RTC_DCHECK(has_value_); - return value_; - } - const T& value() const { - RTC_DCHECK(has_value_); - return value_; - } - T& value() { - RTC_DCHECK(has_value_); - return value_; - } - - // Dereference with a default value in case we don't have a value. - const T& value_or(const T& default_val) const { - // The no-op call prevents the compiler from generating optimized code that - // reads value_ even if !has_value_, but only if FunctionThatDoesNothing is - // not completely inlined; see its declaration.). - return has_value_ ? *optional_internal::FunctionThatDoesNothing(&value_) - : default_val; - } - - // Equality tests. Two Optionals are equal if they contain equivalent values, - // or if they're both empty. - 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 Optional& opt, const T& value) { - return opt.has_value_ && opt.value_ == value; - } - friend bool operator==(const T& value, const Optional& opt) { - return opt.has_value_ && value == opt.value_; - } - - friend bool operator==(const Optional& opt, rtc::nullopt_t) { - return !opt.has_value_; - } - - friend bool operator==(rtc::nullopt_t, const Optional& opt) { - return !opt.has_value_; - } - - 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 Optional& opt, const T& value) { - return !opt.has_value_ || opt.value_ != value; - } - friend bool operator!=(const T& value, const Optional& opt) { - return !opt.has_value_ || value != opt.value_; - } - - friend bool operator!=(const Optional& opt, rtc::nullopt_t) { - return opt.has_value_; - } - - friend bool operator!=(rtc::nullopt_t, const Optional& opt) { - return opt.has_value_; - } - - private: - // Tell sanitizers that value_ shouldn't be touched. - void PoisonValue() { - rtc::AsanPoison(rtc::MakeArrayView(&value_, 1)); - rtc::MsanMarkUninitialized(rtc::MakeArrayView(&value_, 1)); - } - - // Tell sanitizers that value_ is OK to touch again. - void UnpoisonValue() { rtc::AsanUnpoison(rtc::MakeArrayView(&value_, 1)); } - - bool has_value_; // True iff value_ contains a live value. - union { - // empty_ exists only to make it possible to initialize the union, even when - // it doesn't contain any data. If the union goes uninitialized, it may - // trigger compiler warnings. - char empty_; - // By placing value_ in a union, we get to manage its construction and - // destruction manually: the Optional constructors won't automatically - // construct it, and the Optional destructor won't automatically destroy - // it. Basically, this just allocates a properly sized and aligned block of - // memory in which we can manually put a T with placement new. - T value_; - }; -}; - -#ifdef UNIT_TEST -namespace optional_internal { - -// Checks if there's a valid PrintTo(const T&, std::ostream*) call for T. -template -struct HasPrintTo { - private: - struct No {}; - - template - static auto Test(const T2& obj) - -> decltype(PrintTo(obj, std::declval())); - - template - static No Test(...); - - public: - static constexpr bool value = - !std::is_same(std::declval())), No>::value; -}; - -// Checks if there's a valid operator<<(std::ostream&, const T&) call for T. -template -struct HasOstreamOperator { - private: - struct No {}; - - template - static auto Test(const T2& obj) - -> decltype(std::declval() << obj); - - template - static No Test(...); - - public: - static constexpr bool value = - !std::is_same(std::declval())), No>::value; -}; - -// Prefer using PrintTo to print the object. -template -typename std::enable_if::value, void>::type OptionalPrintToHelper( - const T& value, - std::ostream* os) { - PrintTo(value, os); -} - -// Fall back to operator<<(std::ostream&, ...) if it exists. -template -typename std::enable_if::value && !HasPrintTo::value, - void>::type -OptionalPrintToHelper(const T& value, std::ostream* os) { - *os << value; -} - -inline void OptionalPrintObjectBytes(const unsigned char* bytes, - size_t size, - std::ostream* os) { - *os << "(bytes[i]); - } - *os << "]>"; -} - -// As a final back-up, just print the contents of the objcets byte-wise. -template -typename std::enable_if::value && !HasPrintTo::value, - void>::type -OptionalPrintToHelper(const T& value, std::ostream* os) { - OptionalPrintObjectBytes(reinterpret_cast(&value), - sizeof(value), os); -} - -} // namespace optional_internal - -// PrintTo is used by gtest to print out the results of tests. We want to ensure -// the object contained in an Optional can be printed out if it's set, while -// avoiding touching the object's storage if it is undefined. -template -void PrintTo(const rtc::Optional& opt, std::ostream* os) { - if (opt) { - optional_internal::OptionalPrintToHelper(*opt, os); - } else { - *os << ""; - } -} - -#endif // UNIT_TEST +using Optional = absl::optional; } // namespace rtc diff --git a/api/optional_unittest.cc b/api/optional_unittest.cc index ad700dc001..f56dc6d7ac 100644 --- a/api/optional_unittest.cc +++ b/api/optional_unittest.cc @@ -8,6 +8,9 @@ * be found in the AUTHORS file in the root of the source tree. */ +// TODO(bugs.webrtc.org/8821): Delete this file when absl unittests run on +// webrtc bots. + #include #include #include @@ -702,31 +705,16 @@ TEST(OptionalTest, TestDereference) { TEST(OptionalTest, TestDereferenceWithDefault) { auto log = Logger::Setup(); - { - const Logger a(17), b(42); - Optional x(a); - Optional y; - log->push_back("-1-"); - EXPECT_EQ(a, x.value_or(Logger(42))); - log->push_back("-2-"); - EXPECT_EQ(b, y.value_or(Logger(42))); - log->push_back("-3-"); - EXPECT_EQ(a, Optional(Logger(17)).value_or(b)); - log->push_back("-4-"); - EXPECT_EQ(b, Optional().value_or(b)); - log->push_back("-5-"); - } - EXPECT_EQ( - V("0:17. explicit constructor", "1:42. explicit constructor", - "2:17. copy constructor (from 0:17)", "-1-", - "3:42. explicit constructor", "operator== 0:17, 2:17", - "3:42. destructor", "-2-", "4:42. explicit constructor", - "operator== 1:42, 4:42", "4:42. destructor", "-3-", - "5:17. explicit constructor", "6:17. move constructor (from 5:17)", - "operator== 0:17, 6:17", "6:17. destructor", "5:17. destructor", "-4-", - "operator== 1:42, 1:42", "-5-", "2:17. destructor", "1:42. destructor", - "0:17. destructor"), - *log); + const Logger a(17), b(42); + Optional x(a); + Optional y; + EXPECT_EQ(a, x.value_or(Logger(42))); + EXPECT_EQ(b, y.value_or(Logger(42))); + EXPECT_EQ(a, Optional(Logger(17)).value_or(b)); + EXPECT_EQ(b, Optional().value_or(b)); + // Can't expect exact list of constructors and destructors because it is + // compiler-dependent. i.e. msvc produce different output than clang. Calls + // above are subject to copy elision that allow to change behavior. } TEST(OptionalTest, TestEquality) { @@ -871,8 +859,16 @@ TEST(OptionalTest, TestMoveValue) { *log); } -TEST(OptionalTest, TestPrintTo) { - constexpr char kEmptyOptionalMessage[] = ""; +// Nice printing available only when GTEST aware ABSL is present +#ifdef GTEST_HAS_ABSL +#define MaybeTestPrintTo TestPrintTo +#define MaybeTestUnprintablePrintTo TestUnprintablePrintTo +#else +#define MaybeTestPrintTo DISABLED_TestPrintTo +#define MaybeTestUnprintablePrintTo DISABLED_TestUnprintablePrintTo +#endif +TEST(OptionalTest, MaybeTestPrintTo) { + constexpr char kEmptyOptionalMessage[] = "(nullopt)"; const Optional empty_unprintable; const Optional empty_printable; const Optional empty_ostream_printable; @@ -880,14 +876,22 @@ TEST(OptionalTest, TestPrintTo) { EXPECT_EQ(kEmptyOptionalMessage, ::testing::PrintToString(empty_printable)); EXPECT_EQ(kEmptyOptionalMessage, ::testing::PrintToString(empty_ostream_printable)); - EXPECT_NE("1", ::testing::PrintToString(Optional({1}))); - EXPECT_NE("1", ::testing::PrintToString(Optional({1}))); - EXPECT_EQ("The value is 1", + EXPECT_NE("(1)", ::testing::PrintToString(Optional({1}))); + EXPECT_NE("(1)", ::testing::PrintToString(Optional({1}))); + EXPECT_EQ("(The value is 1)", ::testing::PrintToString(Optional({1}))); - EXPECT_EQ("1", + EXPECT_EQ("(1)", ::testing::PrintToString(Optional({1}))); } +TEST(OptionalTest, MaybeTestUnprintablePrintTo) { + struct UnprintableType { + uint8_t value[5]; + }; + Optional opt({0xa1, 0xb2, 0xc3, 0xd4, 0xe5}); + EXPECT_EQ("(5-byte object )", ::testing::PrintToString(opt)); +} + void UnusedFunctionWorkaround() { // These are here to ensure we don't get warnings about ostream and PrintTo // for MyPrintableType never getting called.