From ad9f2050fd8bc46be520359e6dfcc1777fd14482 Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Wed, 26 Sep 2018 11:21:00 -0700 Subject: [PATCH 1/3] Overload Maybe::operator=() to match all copy constructors. Previously we depended on the compiler to implicitly invoke the copy constructor followed by the assignment operator, but this can lead to the compiler making surprising decisions in some cases, such as deciding that the literal 0 is a better match for nullptr than for T. Overloading operator=() to match every copy constructor is the safe thing to do. --- c++/src/kj/common.h | 72 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/c++/src/kj/common.h b/c++/src/kj/common.h index b6be533970..988017f24f 100644 --- a/c++/src/kj/common.h +++ b/c++/src/kj/common.h @@ -1007,6 +1007,52 @@ class NullableValue { return *this; } + inline NullableValue& operator=(T&& other) { emplace(kj::mv(other)); return *this; } + inline NullableValue& operator=(T& other) { emplace(other); return *this; } + inline NullableValue& operator=(const T& other) { emplace(other); return *this; } + inline NullableValue& operator=(const T* other) { + if (other == nullptr) { + *this = nullptr; + } else { + emplace(*other); + } + return *this; + } + template + inline NullableValue& operator=(NullableValue&& other) { + if (other.isSet) { + emplace(kj::mv(other.value)); + } else { + *this = nullptr; + } + return *this; + } + template + inline NullableValue& operator=(const NullableValue& other) { + if (other.isSet) { + emplace(other.value); + } else { + *this = nullptr; + } + return *this; + } + template + inline NullableValue& operator=(const NullableValue& other) { + if (other.isSet) { + emplace(other.value); + } else { + *this = nullptr; + } + return *this; + } + inline NullableValue& operator=(decltype(nullptr)) { + if (isSet) { + isSet = false; + dtor(value); + } + return *this; + } + inline bool operator==(decltype(nullptr)) const { return !isSet; } inline bool operator!=(decltype(nullptr)) const { return isSet; } @@ -1092,10 +1138,36 @@ class Maybe { return ptr.emplace(kj::fwd(params)...); } + inline Maybe& operator=(T&& other) { ptr = kj::mv(other); return *this; } + inline Maybe& operator=(T& other) { ptr = other; return *this; } + inline Maybe& operator=(const T& other) { ptr = other; return *this; } + inline Maybe& operator=(const T* other) { ptr = other; return *this; } + inline Maybe& operator=(Maybe&& other) { ptr = kj::mv(other.ptr); return *this; } inline Maybe& operator=(Maybe& other) { ptr = other.ptr; return *this; } inline Maybe& operator=(const Maybe& other) { ptr = other.ptr; return *this; } + template + Maybe& operator=(Maybe&& other) { + KJ_IF_MAYBE(val, kj::mv(other)) { + ptr.emplace(kj::mv(*val)); + } else { + ptr = nullptr; + } + return *this; + } + template + Maybe& operator=(const Maybe& other) { + KJ_IF_MAYBE(val, other) { + ptr.emplace(*val); + } else { + ptr = nullptr; + } + return *this; + } + + inline Maybe& operator=(decltype(nullptr)) { ptr = nullptr; return *this; } + inline bool operator==(decltype(nullptr)) const { return ptr == nullptr; } inline bool operator!=(decltype(nullptr)) const { return ptr != nullptr; } From d0dea8e50e992f01ed21cec4ebb589f56fe700f7 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Thu, 30 Aug 2018 18:51:39 +0100 Subject: [PATCH 2/3] Test that immovable types are allowed in Maybe Immovable types can be already wrapped into `Maybe`, which can be constructed as default `nullptr`, set to a constructed value via `.emplace()`, but couldn't be cleared back to `nullptr`. The previous commit fixed the bug, this commit adds a test. --- c++/src/kj/common-test.c++ | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/c++/src/kj/common-test.c++ b/c++/src/kj/common-test.c++ index aa84495019..da0b3cf739 100644 --- a/c++/src/kj/common-test.c++ +++ b/c++/src/kj/common-test.c++ @@ -45,6 +45,11 @@ struct ImplicitToInt { } }; +struct Immovable { + Immovable() = default; + KJ_DISALLOW_COPY(Immovable); +}; + TEST(Common, Maybe) { { Maybe m = 123; @@ -63,6 +68,23 @@ TEST(Common, Maybe) { EXPECT_EQ(123, m.orDefault(456)); } + { + Maybe m = 0; + EXPECT_FALSE(m == nullptr); + EXPECT_TRUE(m != nullptr); + KJ_IF_MAYBE(v, m) { + EXPECT_EQ(0, *v); + } else { + ADD_FAILURE(); + } + KJ_IF_MAYBE(v, mv(m)) { + EXPECT_EQ(0, *v); + } else { + ADD_FAILURE(); + } + EXPECT_EQ(0, m.orDefault(456)); + } + { Maybe m = nullptr; EXPECT_TRUE(m == nullptr); @@ -216,6 +238,16 @@ TEST(Common, Maybe) { ADD_FAILURE(); } } + + { + // Test usage of immovable types. + Maybe m; + KJ_EXPECT(m == nullptr); + m.emplace(); + KJ_EXPECT(m != nullptr); + m = nullptr; + KJ_EXPECT(m == nullptr); + } } TEST(Common, MaybeConstness) { From f693e2e57c723f6596af268676668ec38052ac1a Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Thu, 27 Sep 2018 09:59:30 -0700 Subject: [PATCH 3/3] Try to fix MSVC. --- c++/src/kj/common.h | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/c++/src/kj/common.h b/c++/src/kj/common.h index 988017f24f..7a4fa8fa0f 100644 --- a/c++/src/kj/common.h +++ b/c++/src/kj/common.h @@ -870,7 +870,7 @@ class NullableValue { // boolean flag indicating nullness. public: - inline NullableValue(NullableValue&& other) noexcept(noexcept(T(instance()))) + inline NullableValue(NullableValue&& other) : isSet(other.isSet) { if (isSet) { ctor(value, kj::mv(other.value)); @@ -922,8 +922,8 @@ class NullableValue { return value; } - inline NullableValue() noexcept: isSet(false) {} - inline NullableValue(T&& t) noexcept(noexcept(T(instance()))) + inline NullableValue(): isSet(false) {} + inline NullableValue(T&& t) : isSet(true) { ctor(value, kj::mv(t)); } @@ -940,7 +940,7 @@ class NullableValue { if (isSet) ctor(value, *t); } template - inline NullableValue(NullableValue&& other) noexcept(noexcept(T(instance()))) + inline NullableValue(NullableValue&& other) : isSet(other.isSet) { if (isSet) { ctor(value, kj::mv(other.value)); @@ -1106,16 +1106,16 @@ class Maybe { public: Maybe(): ptr(nullptr) {} - Maybe(T&& t) noexcept(noexcept(T(instance()))): ptr(kj::mv(t)) {} + Maybe(T&& t): ptr(kj::mv(t)) {} Maybe(T& t): ptr(t) {} Maybe(const T& t): ptr(t) {} - Maybe(const T* t) noexcept: ptr(t) {} - Maybe(Maybe&& other) noexcept(noexcept(T(instance()))): ptr(kj::mv(other.ptr)) {} + Maybe(const T* t): ptr(t) {} + Maybe(Maybe&& other): ptr(kj::mv(other.ptr)) {} Maybe(const Maybe& other): ptr(other.ptr) {} Maybe(Maybe& other): ptr(other.ptr) {} template - Maybe(Maybe&& other) noexcept(noexcept(T(instance()))) { + Maybe(Maybe&& other) { KJ_IF_MAYBE(val, kj::mv(other)) { ptr.emplace(kj::mv(*val)); } @@ -1127,7 +1127,7 @@ class Maybe { } } - Maybe(decltype(nullptr)) noexcept: ptr(nullptr) {} + Maybe(decltype(nullptr)): ptr(nullptr) {} template inline T& emplace(Params&&... params) { @@ -1252,22 +1252,22 @@ class Maybe { template class Maybe: public DisallowConstCopyIfNotConst { public: - Maybe() noexcept: ptr(nullptr) {} - Maybe(T& t) noexcept: ptr(&t) {} - Maybe(T* t) noexcept: ptr(t) {} + Maybe(): ptr(nullptr) {} + Maybe(T& t): ptr(&t) {} + Maybe(T* t): ptr(t) {} template - inline Maybe(Maybe& other) noexcept: ptr(other.ptr) {} + inline Maybe(Maybe& other): ptr(other.ptr) {} template - inline Maybe(const Maybe& other) noexcept: ptr(const_cast(other.ptr)) {} - inline Maybe(decltype(nullptr)) noexcept: ptr(nullptr) {} + inline Maybe(const Maybe& other): ptr(const_cast(other.ptr)) {} + inline Maybe(decltype(nullptr)): ptr(nullptr) {} - inline Maybe& operator=(T& other) noexcept { ptr = &other; return *this; } - inline Maybe& operator=(T* other) noexcept { ptr = other; return *this; } + inline Maybe& operator=(T& other) { ptr = &other; return *this; } + inline Maybe& operator=(T* other) { ptr = other; return *this; } template - inline Maybe& operator=(Maybe& other) noexcept { ptr = other.ptr; return *this; } + inline Maybe& operator=(Maybe& other) { ptr = other.ptr; return *this; } template - inline Maybe& operator=(const Maybe& other) noexcept { ptr = other.ptr; return *this; } + inline Maybe& operator=(const Maybe& other) { ptr = other.ptr; return *this; } inline bool operator==(decltype(nullptr)) const { return ptr == nullptr; } inline bool operator!=(decltype(nullptr)) const { return ptr != nullptr; }