Skip to content

Commit

Permalink
Make chip::Optional be trivially destructible if the underlying type …
Browse files Browse the repository at this point in the history 
…is. (project-chip#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 <[email protected]>
  • Loading branch information
@andy31415 @andreilitvin
andy31415 and andreilitvin authored Jan 10, 2025
1 parent a96f6ca commit 448930e
Show file tree
Hide file tree
Showing 4 changed files with 105 additions and 72 deletions.
1 change: 0 additions & 1 deletion ...gement-app/energy-management-common/device-energy-management/src/DEMTestEventTriggers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<chip::app::Clusters::DeviceEnergyManagement::Structs::ForecastStruct::Type> sForecast;

chip::app::Clusters::DeviceEnergyManagement::Structs::PowerAdjustStruct::Type sPowerAdjustments[MAX_POWER_ADJUSTMENTS];
chip::app::Clusters::DeviceEnergyManagement::Structs::PowerAdjustCapabilityStruct::Type sPowerAdjustCapabilityStruct;
Expand Down
1 change: 0 additions & 1 deletion ...lusters/valve-configuration-and-control-server/valve-configuration-and-control-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,6 @@ CHIP_ERROR CloseValve(EndpointId ep)
CHIP_ERROR SetValveLevel(EndpointId ep, DataModel::Nullable<Percent> level, DataModel::Nullable<uint32_t> openDuration)
{
Delegate * delegate = GetDelegate(ep);
Optional<Status> status = Optional<Status>::Missing();
CHIP_ERROR attribute_error = CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute);

if (HasFeature(ep, ValveConfigurationAndControl::Feature::kTimeSync))
Expand Down
163 changes: 99 additions & 64 deletions src/lib/core/Optional.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,92 +48,92 @@ template <class T>
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 <class... Args>
constexpr explicit Optional(InPlaceType, Args &&... args) : mHasValue(true)
constexpr explicit Optional(InPlaceType, Args &&... args)
{
new (&mValue.mData) T(std::forward<Args>(args)...);
mValueHolder.mHasValue = true;
new (&mValueHolder.mValue.mData) T(std::forward<Args>(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 <class U, std::enable_if_t<!std::is_same_v<T, U> && std::is_convertible_v<const U, T>, bool> = true>
constexpr Optional(const Optional<U> & other) : mHasValue(other.HasValue())
constexpr Optional(const Optional<U> & other)
{
if (mHasValue)
mValueHolder.mHasValue = other.HasValue();
if (mValueHolder.mHasValue)
{
new (&mValue.mData) T(other.Value());
new (&mValueHolder.mValue.mData) T(other.Value());
}
}

// Converts an Optional of a type that requires explicit conversion
template <class U,
std::enable_if_t<!std::is_same_v<T, U> && !std::is_convertible_v<const U, T> && std::is_constructible_v<T, const U &>,
bool> = true>
constexpr explicit Optional(const Optional<U> & other) : mHasValue(other.HasValue())
constexpr explicit Optional(const Optional<U> & 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;
}
Expand All @@ -142,24 +142,24 @@ class Optional
template <class... Args>
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>(args)...);
return mValue.mData;
mValueHolder.mHasValue = true;
new (&mValueHolder.mValue.mData) T(std::forward<Args>(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<T> & value)
Expand All @@ -177,48 +177,49 @@ 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;
otherwise returns the provided default value. */
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; }
Expand All @@ -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<std::is_trivially_destructible_v<T>, TrivialDestructor, NonTrivialDestructor>
{
Value() {}
~Value() {}
T mData;
} mValue;
};

ValueHolder mValueHolder;
};

template <class T>
Expand Down
12 changes: 6 additions & 6 deletions src/lib/core/tests/TestOptional.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,18 @@
* limitations under the License.
*/

/**
* @file
* This file implements a test for CHIP core library reference counted object.
*
*/

#include <array>
#include <cinttypes>
#include <cstdint>
#include <cstring>
#include <string>

#include <pw_unit_test/framework.h>

#include <lib/core/Optional.h>
#include <lib/core/StringBuilderAdapters.h>
#include <lib/support/Span.h>
#include <type_traits>

using namespace chip;

Expand Down Expand Up @@ -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<chip::Optional<unsigned>>);
static_assert(!std::is_trivially_destructible_v<chip::Optional<std::string>>);

TEST(TestOptional, TestBasic)
{
// Set up our test Count objects, which will mess with counts, before we reset the
Expand Down

0 comments on commit 448930e

Please sign in to comment.