From 448930e8bec153d359cd75b44a86d734f0877107 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 10 Jan 2025 17:31:29 -0500 Subject: [PATCH] Make chip::Optional be trivially destructible if the underlying type is. (#37017) * Make chip::Optional be trivially destructible if the underlying type is. Previous implementation always had a destructor, so it was never trivially destructible. * Remove extra char * Remove unused variable * Remove one more unused variable --------- Co-authored-by: Andrei Litvin --- .../src/DEMTestEventTriggers.cpp | 1 - ...valve-configuration-and-control-server.cpp | 1 - src/lib/core/Optional.h | 163 +++++++++++------- src/lib/core/tests/TestOptional.cpp | 12 +- 4 files changed, 105 insertions(+), 72 deletions(-) diff --git a/examples/energy-management-app/energy-management-common/device-energy-management/src/DEMTestEventTriggers.cpp b/examples/energy-management-app/energy-management-common/device-energy-management/src/DEMTestEventTriggers.cpp index 23ed2afe1b994d..07b12df957c8d1 100644 --- a/examples/energy-management-app/energy-management-common/device-energy-management/src/DEMTestEventTriggers.cpp +++ b/examples/energy-management-app/energy-management-common/device-energy-management/src/DEMTestEventTriggers.cpp @@ -36,7 +36,6 @@ constexpr uint16_t MAX_POWER_ADJUSTMENTS = 5; chip::app::Clusters::DeviceEnergyManagement::Structs::SlotStruct::Type sSlots[MAX_SLOTS]; chip::app::Clusters::DeviceEnergyManagement::Structs::ForecastStruct::Type sForecastStruct; -chip::app::DataModel::Nullable sForecast; chip::app::Clusters::DeviceEnergyManagement::Structs::PowerAdjustStruct::Type sPowerAdjustments[MAX_POWER_ADJUSTMENTS]; chip::app::Clusters::DeviceEnergyManagement::Structs::PowerAdjustCapabilityStruct::Type sPowerAdjustCapabilityStruct; diff --git a/src/app/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.cpp b/src/app/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.cpp index fd89ab135d384c..7ebeebc9de78fb 100644 --- a/src/app/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.cpp +++ b/src/app/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.cpp @@ -309,7 +309,6 @@ CHIP_ERROR CloseValve(EndpointId ep) CHIP_ERROR SetValveLevel(EndpointId ep, DataModel::Nullable level, DataModel::Nullable openDuration) { Delegate * delegate = GetDelegate(ep); - Optional status = Optional::Missing(); CHIP_ERROR attribute_error = CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute); if (HasFeature(ep, ValveConfigurationAndControl::Feature::kTimeSync)) diff --git a/src/lib/core/Optional.h b/src/lib/core/Optional.h index a8f544ce1d9819..eb661609f31908 100644 --- a/src/lib/core/Optional.h +++ b/src/lib/core/Optional.h @@ -48,41 +48,39 @@ template class Optional { public: - constexpr Optional() : mHasValue(false) {} - constexpr Optional(NullOptionalType) : mHasValue(false) {} + constexpr Optional() {} + constexpr Optional(NullOptionalType) {} - ~Optional() + explicit Optional(const T & value) { - // NOLINTNEXTLINE(clang-analyzer-core.uninitialized.Branch): mData is set when mHasValue - if (mHasValue) - { - mValue.mData.~T(); - } + mValueHolder.mHasValue = true; + new (&mValueHolder.mValue.mData) T(value); } - explicit Optional(const T & value) : mHasValue(true) { new (&mValue.mData) T(value); } - template - constexpr explicit Optional(InPlaceType, Args &&... args) : mHasValue(true) + constexpr explicit Optional(InPlaceType, Args &&... args) { - new (&mValue.mData) T(std::forward(args)...); + mValueHolder.mHasValue = true; + new (&mValueHolder.mValue.mData) T(std::forward(args)...); } - constexpr Optional(const Optional & other) : mHasValue(other.mHasValue) + constexpr Optional(const Optional & other) { - if (mHasValue) + mValueHolder.mHasValue = other.mValueHolder.mHasValue; + if (mValueHolder.mHasValue) { - new (&mValue.mData) T(other.mValue.mData); + new (&mValueHolder.mValue.mData) T(other.mValueHolder.mValue.mData); } } // Converts an Optional of an implicitly convertible type template && std::is_convertible_v, bool> = true> - constexpr Optional(const Optional & other) : mHasValue(other.HasValue()) + constexpr Optional(const Optional & other) { - if (mHasValue) + mValueHolder.mHasValue = other.HasValue(); + if (mValueHolder.mHasValue) { - new (&mValue.mData) T(other.Value()); + new (&mValueHolder.mValue.mData) T(other.Value()); } } @@ -90,50 +88,52 @@ class Optional template && !std::is_convertible_v && std::is_constructible_v, bool> = true> - constexpr explicit Optional(const Optional & other) : mHasValue(other.HasValue()) + constexpr explicit Optional(const Optional & other) { - if (mHasValue) + mValueHolder.mHasValue = other.HasValue(); + if (mValueHolder.mHasValue) { - new (&mValue.mData) T(other.Value()); + new (&mValueHolder.mValue.mData) T(other.Value()); } } - constexpr Optional(Optional && other) : mHasValue(other.mHasValue) + constexpr Optional(Optional && other) { - if (mHasValue) + mValueHolder.mHasValue = other.mValueHolder.mHasValue; + if (mValueHolder.mHasValue) { - new (&mValue.mData) T(std::move(other.mValue.mData)); - other.mValue.mData.~T(); - other.mHasValue = false; + new (&mValueHolder.mValue.mData) T(std::move(other.mValueHolder.mValue.mData)); + other.mValueHolder.mValue.mData.~T(); + other.mValueHolder.mHasValue = false; } } constexpr Optional & operator=(const Optional & other) { - if (mHasValue) + if (mValueHolder.mHasValue) { - mValue.mData.~T(); + mValueHolder.mValue.mData.~T(); } - mHasValue = other.mHasValue; - if (mHasValue) + mValueHolder.mHasValue = other.mValueHolder.mHasValue; + if (mValueHolder.mHasValue) { - new (&mValue.mData) T(other.mValue.mData); + new (&mValueHolder.mValue.mData) T(other.mValueHolder.mValue.mData); } return *this; } constexpr Optional & operator=(Optional && other) { - if (mHasValue) + if (mValueHolder.mHasValue) { - mValue.mData.~T(); + mValueHolder.mValue.mData.~T(); } - mHasValue = other.mHasValue; - if (mHasValue) + mValueHolder.mHasValue = other.mValueHolder.mHasValue; + if (mValueHolder.mHasValue) { - new (&mValue.mData) T(std::move(other.mValue.mData)); - other.mValue.mData.~T(); - other.mHasValue = false; + new (&mValueHolder.mValue.mData) T(std::move(other.mValueHolder.mValue.mData)); + other.mValueHolder.mValue.mData.~T(); + other.mValueHolder.mHasValue = false; } return *this; } @@ -142,24 +142,24 @@ class Optional template constexpr T & Emplace(Args &&... args) { - if (mHasValue) + if (mValueHolder.mHasValue) { - mValue.mData.~T(); + mValueHolder.mValue.mData.~T(); } - mHasValue = true; - new (&mValue.mData) T(std::forward(args)...); - return mValue.mData; + mValueHolder.mHasValue = true; + new (&mValueHolder.mValue.mData) T(std::forward(args)...); + return mValueHolder.mValue.mData; } /** Make the optional contain a specific value */ constexpr void SetValue(const T & value) { - if (mHasValue) + if (mValueHolder.mHasValue) { - mValue.mData.~T(); + mValueHolder.mValue.mData.~T(); } - mHasValue = true; - new (&mValue.mData) T(value); + mValueHolder.mHasValue = true; + new (&mValueHolder.mValue.mData) T(value); } constexpr void SetValue(std::optional & value) @@ -177,36 +177,36 @@ class Optional /** Make the optional contain a specific value */ constexpr void SetValue(T && value) { - if (mHasValue) + if (mValueHolder.mHasValue) { - mValue.mData.~T(); + mValueHolder.mValue.mData.~T(); } - mHasValue = true; - new (&mValue.mData) T(std::move(value)); + mValueHolder.mHasValue = true; + new (&mValueHolder.mValue.mData) T(std::move(value)); } /** Invalidate the value inside the optional. Optional now has no value */ constexpr void ClearValue() { - if (mHasValue) + if (mValueHolder.mHasValue) { - mValue.mData.~T(); + mValueHolder.mValue.mData.~T(); } - mHasValue = false; + mValueHolder.mHasValue = false; } /** Gets the current value of the optional. Valid IFF `HasValue`. */ T & Value() & { VerifyOrDie(HasValue()); - return mValue.mData; + return mValueHolder.mValue.mData; } /** Gets the current value of the optional. Valid IFF `HasValue`. */ const T & Value() const & { VerifyOrDie(HasValue()); - return mValue.mData; + return mValueHolder.mValue.mData; } /** Gets the current value of the optional if the optional has a value; @@ -214,11 +214,12 @@ class Optional const T & ValueOr(const T & defaultValue) const { return HasValue() ? Value() : defaultValue; } /** Checks if the optional contains a value or not */ - constexpr bool HasValue() const { return mHasValue; } + constexpr bool HasValue() const { return mValueHolder.mHasValue; } bool operator==(const Optional & other) const { - return (mHasValue == other.mHasValue) && (!other.mHasValue || (mValue.mData == other.mValue.mData)); + return (mValueHolder.mHasValue == other.mValueHolder.mHasValue) && + (!other.mValueHolder.mHasValue || (mValueHolder.mValue.mData == other.mValueHolder.mValue.mData)); } bool operator!=(const Optional & other) const { return !(*this == other); } bool operator==(const T & other) const { return HasValue() && Value() == other; } @@ -241,13 +242,47 @@ class Optional } private: - bool mHasValue; - union Value + // A container of bool + value (without constructor/destructor) when the underlying + // type has a trivial destructor + class TrivialDestructor + { + public: + bool mHasValue = false; + union Value + { + Value() {} + T mData; + } mValue; + }; + + // A container of bool + value that destroys the underlying type when mHasValue is true. + // To be used for non-trivial destructor classes. + class NonTrivialDestructor + { + public: + ~NonTrivialDestructor() + { + // NOLINTNEXTLINE(clang-analyzer-core.uninitialized.Branch): mData is set when mHasValue + if (mHasValue) + { + mValue.mData.~T(); + } + } + + bool mHasValue = false; + union Value + { + Value() {} + ~Value() {} + T mData; + } mValue; + }; + + class ValueHolder : public std::conditional_t, TrivialDestructor, NonTrivialDestructor> { - Value() {} - ~Value() {} - T mData; - } mValue; + }; + + ValueHolder mValueHolder; }; template diff --git a/src/lib/core/tests/TestOptional.cpp b/src/lib/core/tests/TestOptional.cpp index e57657464369bd..2e2a543ac10b60 100644 --- a/src/lib/core/tests/TestOptional.cpp +++ b/src/lib/core/tests/TestOptional.cpp @@ -17,22 +17,18 @@ * limitations under the License. */ -/** - * @file - * This file implements a test for CHIP core library reference counted object. - * - */ - #include #include #include #include +#include #include #include #include #include +#include using namespace chip; @@ -76,6 +72,10 @@ struct CountMovable : public Count int Count::created; int Count::destroyed; +// Optional is trivially destructible if the underlying type is trivially destructible +static_assert(std::is_trivially_destructible_v>); +static_assert(!std::is_trivially_destructible_v>); + TEST(TestOptional, TestBasic) { // Set up our test Count objects, which will mess with counts, before we reset the