From d040480f69cc6fe65dd101c493d0561a0cdbaa4a Mon Sep 17 00:00:00 2001 From: kwiberg Date: Mon, 9 May 2016 06:06:05 -0700 Subject: [PATCH] rtc::Optional: Don't secretly contain a default-constructed T when empty Instead, use a neat trick with union to ensure that we have a T only when we're supposed to (and just a bunch of unused memory otherwise). This is how std::optional behaves, so it makes sense for us to do the same (and it's convenient, too, since we don't have to pay for the default-constructed T, and we support types that don't have default constructors). Doing this became possible recently when we dropped support for MSVC 2013, which didn't support unions containing non-trivial types. Review-Url: https://codereview.webrtc.org/1896833004 Cr-Commit-Position: refs/heads/master@{#12664} --- webrtc/base/optional.h | 110 +++++++++++++++++++++++++------ webrtc/base/optional_unittest.cc | 106 +++++++++++++---------------- 2 files changed, 134 insertions(+), 82 deletions(-) diff --git a/webrtc/base/optional.h b/webrtc/base/optional.h index 7320533c63..25cfbfe417 100644 --- a/webrtc/base/optional.h +++ b/webrtc/base/optional.h @@ -19,11 +19,7 @@ namespace rtc { -// Simple std::experimental::optional-wannabe. It either contains a T or not. -// In order to keep the implementation simple and portable, this implementation -// actually contains a (default-constructed) T even when it supposedly doesn't -// contain a value; use e.g. std::unique_ptr instead if that's too -// expensive. +// 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 @@ -66,21 +62,90 @@ class Optional final { Optional() : has_value_(false) {} // Construct an Optional that contains a value. - explicit Optional(const T& val) : value_(val), has_value_(true) {} - explicit Optional(T&& val) : value_(std::move(val)), has_value_(true) {} + explicit Optional(const T& value) : has_value_(true) { + new (&value_) T(value); + } + explicit Optional(T&& value) : has_value_(true) { + new (&value_) T(std::move(value)); + } - // Copy and move constructors. - Optional(const Optional&) = default; - Optional(Optional&&) = default; + // 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_); + } - // Assignment. - Optional& operator=(const Optional&) = default; - Optional& operator=(Optional&&) = default; + // 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_)); + } + ~Optional() { + if (has_value_) + value_.~T(); + } + + // 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 { + new (&value_) T(m.value_); // T's copy constructor. + has_value_ = true; + } + } else if (has_value_) { + value_.~T(); + has_value_ = false; + } + 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 { + new (&value_) T(std::move(m.value_)); // T's move constructor. + has_value_ = true; + } + } else if (has_value_) { + value_.~T(); + has_value_ = false; + } + 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) { - using std::swap; - swap(m1.value_, m2.value_); - swap(m1.has_value_, m2.has_value_); + 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. + new (&m2.value_) T(std::move(m1.value_)); + m1.value_.~T(); // Destroy the moved-from value. + m1.has_value_ = false; + m2.has_value_ = true; + } + } else if (m2.has_value_) { + // Only m2 has a value: move it to m1. + new (&m1.value_) T(std::move(m2.value_)); + m2.value_.~T(); // Destroy the moved-from value. + m1.has_value_ = true; + m2.has_value_ = false; + } } // Conversion to bool to test if we have a value. @@ -122,10 +187,15 @@ class Optional final { } private: - // Invariant: Unless *this has been moved from, value_ is default-initialized - // (or copied or moved from a default-initialized T) if !has_value_. - T value_; - bool has_value_; + bool has_value_; // True iff value_ contains a live value. + union { + // 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_; + }; }; } // namespace rtc diff --git a/webrtc/base/optional_unittest.cc b/webrtc/base/optional_unittest.cc index af82b9232c..b51701f6b2 100644 --- a/webrtc/base/optional_unittest.cc +++ b/webrtc/base/optional_unittest.cc @@ -125,7 +125,7 @@ TEST(OptionalTest, TestConstructDefault) { Optional x; EXPECT_FALSE(x); } - EXPECT_EQ(V("0:0. default constructor", "0:0. destructor"), *log); + EXPECT_EQ(V(), *log); } TEST(OptionalTest, TestConstructCopyEmpty) { @@ -136,9 +136,7 @@ TEST(OptionalTest, TestConstructCopyEmpty) { auto y = x; EXPECT_FALSE(y); } - EXPECT_EQ(V("0:0. default constructor", "1:0. copy constructor (from 0:0)", - "1:0. destructor", "0:0. destructor"), - *log); + EXPECT_EQ(V(), *log); } TEST(OptionalTest, TestConstructCopyFull) { @@ -166,9 +164,7 @@ TEST(OptionalTest, TestConstructMoveEmpty) { auto y = std::move(x); EXPECT_FALSE(y); } - EXPECT_EQ(V("0:0. default constructor", "1:0. move constructor (from 0:0)", - "1:0. destructor", "0:0. destructor"), - *log); + EXPECT_EQ(V(), *log); } TEST(OptionalTest, TestConstructMoveFull) { @@ -195,10 +191,7 @@ TEST(OptionalTest, TestCopyAssignToEmptyFromEmpty) { Optional x, y; x = y; } - EXPECT_EQ( - V("0:0. default constructor", "1:1. default constructor", - "0:1. operator= copy (from 1:1)", "1:1. destructor", "0:1. destructor"), - *log); + EXPECT_EQ(V(), *log); } TEST(OptionalTest, TestCopyAssignToFullFromEmpty) { @@ -212,9 +205,7 @@ TEST(OptionalTest, TestCopyAssignToFullFromEmpty) { } EXPECT_EQ( V("0:17. explicit constructor", "1:17. move constructor (from 0:17)", - "0:17. destructor", "2:2. default constructor", "---", - "1:2. operator= copy (from 2:2)", "---", "2:2. destructor", - "1:2. destructor"), + "0:17. destructor", "---", "1:17. destructor", "---"), *log); } @@ -227,11 +218,11 @@ TEST(OptionalTest, TestCopyAssignToEmptyFromFull) { x = y; log->push_back("---"); } - EXPECT_EQ(V("0:0. default constructor", "1:17. explicit constructor", - "2:17. move constructor (from 1:17)", "1:17. destructor", "---", - "0:17. operator= copy (from 2:17)", "---", "2:17. destructor", - "0:17. destructor"), - *log); + EXPECT_EQ( + V("0:17. explicit constructor", "1:17. move constructor (from 0:17)", + "0:17. destructor", "---", "2:17. copy constructor (from 1:17)", "---", + "1:17. destructor", "2:17. destructor"), + *log); } TEST(OptionalTest, TestCopyAssignToFullFromFull) { @@ -261,10 +252,10 @@ TEST(OptionalTest, TestCopyAssignToEmptyFromT) { x = Optional(y); log->push_back("---"); } - EXPECT_EQ(V("0:0. default constructor", "1:17. explicit constructor", "---", - "2:17. copy constructor (from 1:17)", - "0:17. operator= move (from 2:17)", "2:17. destructor", "---", - "1:17. destructor", "0:17. destructor"), + EXPECT_EQ(V("0:17. explicit constructor", "---", + "1:17. copy constructor (from 0:17)", + "2:17. move constructor (from 1:17)", "1:17. destructor", "---", + "0:17. destructor", "2:17. destructor"), *log); } @@ -292,10 +283,7 @@ TEST(OptionalTest, TestMoveAssignToEmptyFromEmpty) { Optional x, y; x = std::move(y); } - EXPECT_EQ( - V("0:0. default constructor", "1:1. default constructor", - "0:1. operator= move (from 1:1)", "1:1. destructor", "0:1. destructor"), - *log); + EXPECT_EQ(V(), *log); } TEST(OptionalTest, TestMoveAssignToFullFromEmpty) { @@ -309,9 +297,7 @@ TEST(OptionalTest, TestMoveAssignToFullFromEmpty) { } EXPECT_EQ( V("0:17. explicit constructor", "1:17. move constructor (from 0:17)", - "0:17. destructor", "2:2. default constructor", "---", - "1:2. operator= move (from 2:2)", "---", "2:2. destructor", - "1:2. destructor"), + "0:17. destructor", "---", "1:17. destructor", "---"), *log); } @@ -324,11 +310,11 @@ TEST(OptionalTest, TestMoveAssignToEmptyFromFull) { x = std::move(y); log->push_back("---"); } - EXPECT_EQ(V("0:0. default constructor", "1:17. explicit constructor", - "2:17. move constructor (from 1:17)", "1:17. destructor", "---", - "0:17. operator= move (from 2:17)", "---", "2:17. destructor", - "0:17. destructor"), - *log); + EXPECT_EQ( + V("0:17. explicit constructor", "1:17. move constructor (from 0:17)", + "0:17. destructor", "---", "2:17. move constructor (from 1:17)", "---", + "1:17. destructor", "2:17. destructor"), + *log); } TEST(OptionalTest, TestMoveAssignToFullFromFull) { @@ -358,10 +344,10 @@ TEST(OptionalTest, TestMoveAssignToEmptyFromT) { x = Optional(std::move(y)); log->push_back("---"); } - EXPECT_EQ(V("0:0. default constructor", "1:17. explicit constructor", "---", - "2:17. move constructor (from 1:17)", - "0:17. operator= move (from 2:17)", "2:17. destructor", "---", - "1:17. destructor", "0:17. destructor"), + EXPECT_EQ(V("0:17. explicit constructor", "---", + "1:17. move constructor (from 0:17)", + "2:17. move constructor (from 1:17)", "1:17. destructor", "---", + "0:17. destructor", "2:17. destructor"), *log); } @@ -426,14 +412,13 @@ TEST(OptionalTest, TestDereferenceWithDefault) { } EXPECT_EQ( V("0:17. explicit constructor", "1:42. explicit constructor", - "2:17. copy constructor (from 0:17)", "3:3. default constructor", "-1-", - "4:42. explicit constructor", "operator== 0:17, 2:17", - "4:42. destructor", "-2-", "5:42. explicit constructor", - "operator== 1:42, 5:42", "5:42. destructor", "-3-", - "6:17. explicit constructor", "7:17. move constructor (from 6:17)", - "operator== 0:17, 7:17", "7:17. destructor", "6:17. destructor", "-4-", - "8:8. default constructor", "operator== 1:42, 1:42", "8:8. destructor", - "-5-", "3:3. destructor", "2:17. destructor", "1:42. destructor", + "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); } @@ -452,16 +437,15 @@ TEST(OptionalTest, TestEquality) { EXPECT_EQ(me1, me2); log->push_back("---"); } - EXPECT_EQ(V("0:17. explicit constructor", "1:42. explicit constructor", - "2:17. copy constructor (from 0:17)", - "3:17. copy constructor (from 0:17)", - "4:42. copy constructor (from 1:42)", "5:5. default constructor", - "6:6. default constructor", "---", "operator== 2:17, 2:17", - "operator== 2:17, 3:17", "operator!= 2:17, 4:42", "---", - "6:6. destructor", "5:5. destructor", "4:42. destructor", - "3:17. destructor", "2:17. destructor", "1:42. destructor", - "0:17. destructor"), - *log); + EXPECT_EQ( + V("0:17. explicit constructor", "1:42. explicit constructor", + "2:17. copy constructor (from 0:17)", + "3:17. copy constructor (from 0:17)", + "4:42. copy constructor (from 1:42)", "---", "operator== 2:17, 2:17", + "operator== 2:17, 3:17", "operator!= 2:17, 4:42", "---", + "4:42. destructor", "3:17. destructor", "2:17. destructor", + "1:42. destructor", "0:17. destructor"), + *log); } TEST(OptionalTest, TestSwap) { @@ -478,11 +462,9 @@ TEST(OptionalTest, TestSwap) { EXPECT_EQ(V("0:17. explicit constructor", "1:42. explicit constructor", "2:17. copy constructor (from 0:17)", "3:42. copy constructor (from 1:42)", - "4:17. copy constructor (from 0:17)", "5:5. default constructor", - "6:6. default constructor", "7:7. default constructor", "---", - "swap 2:42, 3:17", "swap 4:5, 5:17", "swap 6:7, 7:6", "---", - "7:6. destructor", "6:7. destructor", "5:17. destructor", - "4:5. destructor", "3:17. destructor", "2:42. destructor", + "4:17. copy constructor (from 0:17)", "---", "swap 2:42, 3:17", + "5:17. move constructor (from 4:17)", "4:17. destructor", "---", + "5:17. destructor", "3:17. destructor", "2:42. destructor", "1:42. destructor", "0:17. destructor"), *log); }