Use std::optional for chip::Nullable (#33080)

* make nullable depend on std::optional instead of chip::optional * Fix equality operator * Fix inequality operator as well * Fix test: previous code was enforcing chip::optional behavior on in-place construction * Restyle * Fix some compile errors ... std::optional now detects unused variables * Restyle * Remove unused variable * Restyle * Review updates * Review fix * Typo fixes * Ugly forward of Value and ValueOr .... however at least hopefully it is complete * Do not expose value/value_or at this time and keep Value/ValueOr as the public API for nullable * Fix copy and paste typo * Make more changes to make things compile and work like the old nullable * Use value again instead of operator** ... I was looking at the wrong compiler errors * Undo unrelated change * Make clang-tidy be abel to compile ... apparently it really does not like using has_value in equality operators * Restyle * Another update to make clang-tidy happy for the other equality operator * Update src/app/data-model/Nullable.h Co-authored-by: Boris Zbarsky <[email protected]> * Undo odd whitespace change --------- Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Apr 29, 2024 · dfe64ee · dfe64ee
1 parent 9eb2e29
commit dfe64ee
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 32 deletions.
diff --git a/...lusters/valve-configuration-and-control-server/valve-configuration-and-control-server.cpp b/...lusters/valve-configuration-and-control-server/valve-configuration-and-control-server.cpp
@@ -277,8 +277,7 @@ Delegate * GetDefaultDelegate(EndpointId endpoint)
 
 CHIP_ERROR CloseValve(EndpointId ep)
 {
-    Delegate * delegate = GetDelegate(ep);
-    DataModel::Nullable<uint32_t> rDuration;
+    Delegate * delegate        = GetDelegate(ep);
     CHIP_ERROR attribute_error = CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute);
 
     VerifyOrReturnError(Status::Success == TargetState::Set(ep, ValveConfigurationAndControl::ValveStateEnum::kClosed),
@@ -309,10 +308,8 @@ 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();
-    DataModel::Nullable<Percent> openLevel;
-    DataModel::Nullable<uint64_t> autoCloseTime;
+    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))
@@ -328,6 +325,7 @@ CHIP_ERROR SetValveLevel(EndpointId ep, DataModel::Nullable<Percent> level, Data
             VerifyOrReturnError(UnixEpochToChipEpochMicros(utcTime.count(), chipEpochTime), CHIP_ERROR_INVALID_TIME);
 
             uint64_t time = openDuration.Value() * chip::kMicrosecondsPerSecond;
+            DataModel::Nullable<uint64_t> autoCloseTime;
             autoCloseTime.SetNonNull(chipEpochTime + time);
             VerifyOrReturnError(Status::Success == AutoCloseTime::Set(ep, autoCloseTime), attribute_error);
         }

diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h
@@ -19,9 +19,9 @@
 #pragma once
 
 #include <app/util/attribute-storage-null-handling.h>
-#include <lib/core/Optional.h>
-
+#include <optional>
 #include <type_traits>
+#include <utility>
 
 namespace chip {
 namespace app {
@@ -37,31 +37,40 @@ inline constexpr auto NullNullable = NullOptional;
  * things.
  */
 template <typename T>
-struct Nullable : protected Optional<T>
+struct Nullable : protected std::optional<T>
 {
+
     //
     // The following 'using' statement is needed to make visible
     // all constructors of the base class within this derived class.
     //
-    using Optional<T>::Optional;
+    using std::optional<T>::optional;
+    using std::optional<T>::operator*;
+    using std::optional<T>::operator->;
 
-    // Pull in APIs that make sense on Nullable with the same names as on
-    // Optional.
-    using Optional<T>::Value;
-    using Optional<T>::ValueOr;
+    Nullable(NullOptionalType) : std::optional<T>(std::nullopt) {}
 
     // Some consumers need an easy way to determine our underlying type.
     using UnderlyingType = T;
 
-    constexpr void SetNull() { Optional<T>::ClearValue(); }
-    constexpr bool IsNull() const { return !Optional<T>::HasValue(); }
+    constexpr void SetNull() { std::optional<T>::reset(); }
+    constexpr bool IsNull() const { return !std::optional<T>::has_value(); }
 
     template <class... Args>
     constexpr T & SetNonNull(Args &&... args)
     {
-        return Optional<T>::Emplace(std::forward<Args>(args)...);
+        return std::optional<T>::emplace(std::forward<Args>(args)...);
+    }
+
+    template <typename... Args>
+    constexpr auto ValueOr(Args &&... args) const
+    {
+        return std::optional<T>::value_or(std::forward<Args>(args)...);
     }
 
+    inline constexpr const T & Value() const { return std::optional<T>::value(); }
+    inline T & Value() { return std::optional<T>::value(); }
+
     // For integer types, being nullable involves a range restriction.
     template <
         typename U = std::decay_t<T>,
@@ -96,22 +105,26 @@ struct Nullable : protected Optional<T>
     // The only fabric-scoped objects in the spec are commands, events and structs inside lists, and none of those can be nullable.
     static constexpr bool kIsFabricScoped = false;
 
-    bool operator==(const Nullable & other) const { return Optional<T>::operator==(other); }
-    bool operator!=(const Nullable & other) const { return Optional<T>::operator!=(other); }
-    bool operator==(const T & other) const { return Optional<T>::operator==(other); }
-    bool operator!=(const T & other) const { return Optional<T>::operator!=(other); }
+    inline bool operator==(const T & other) const { return static_cast<const std::optional<T> &>(*this) == other; }
+    inline bool operator!=(const T & other) const { return !(*this == other); }
+
+    inline bool operator==(const Nullable<T> & other) const
+    {
+        return static_cast<const std::optional<T> &>(*this) == static_cast<const std::optional<T> &>(other);
+    }
+    inline bool operator!=(const Nullable<T> & other) const { return !(*this == other); }
 };
 
 template <class T>
 constexpr Nullable<std::decay_t<T>> MakeNullable(T && value)
 {
-    return Nullable<std::decay_t<T>>(InPlace, std::forward<T>(value));
+    return Nullable<std::decay_t<T>>(std::in_place, std::forward<T>(value));
 }
 
 template <class T, class... Args>
 constexpr Nullable<T> MakeNullable(Args &&... args)
 {
-    return Nullable<T>(InPlace, std::forward<Args>(args)...);
+    return Nullable<T>(std::in_place, std::forward<Args>(args)...);
 }
 
 } // namespace DataModel

diff --git a/src/app/tests/TestNullable.cpp b/src/app/tests/TestNullable.cpp
@@ -45,6 +45,7 @@ struct CtorDtorCounter
     CtorDtorCounter & operator=(CtorDtorCounter &&) = default;
 
     bool operator==(const CtorDtorCounter & o) const { return m == o.m; }
+    bool operator!=(const CtorDtorCounter & o) const { return m != o.m; }
 
     int m;
 
@@ -68,6 +69,9 @@ struct MovableCtorDtorCounter : public CtorDtorCounter
 
     MovableCtorDtorCounter(MovableCtorDtorCounter && o)           = default;
     MovableCtorDtorCounter & operator=(MovableCtorDtorCounter &&) = default;
+
+    using CtorDtorCounter::operator==;
+    using CtorDtorCounter::operator!=;
 };
 
 int CtorDtorCounter::created   = 0;
@@ -165,24 +169,26 @@ static void TestMove(nlTestSuite * inSuite, void * inContext)
     CtorDtorCounter::ResetCounter();
 
     {
-        auto testSrc = MakeNullable<MovableCtorDtorCounter>(400);
-        Nullable<MovableCtorDtorCounter> testDst(std::move(testSrc));
-        NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 2 && CtorDtorCounter::destroyed == 1);
+        auto testSrc = MakeNullable<MovableCtorDtorCounter>(400);     // construct
+        Nullable<MovableCtorDtorCounter> testDst(std::move(testSrc)); // move construct
+        NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 2 && CtorDtorCounter::destroyed == 0);
         NL_TEST_ASSERT(inSuite, !testDst.IsNull() && testDst.Value().m == 400);
+        // destroy both testsSrc and testDst
     }
     NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 2 && CtorDtorCounter::destroyed == 2);
 
+    CtorDtorCounter::ResetCounter();
     {
-        Nullable<MovableCtorDtorCounter> testDst;
-        NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 2 && CtorDtorCounter::destroyed == 2);
+        Nullable<MovableCtorDtorCounter> testDst; // no object construction
+        NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 0 && CtorDtorCounter::destroyed == 0);
         NL_TEST_ASSERT(inSuite, !!testDst.IsNull());
 
-        auto testSrc = MakeNullable<MovableCtorDtorCounter>(401);
-        testDst      = std::move(testSrc);
-        NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 4 && CtorDtorCounter::destroyed == 3);
+        auto testSrc = MakeNullable<MovableCtorDtorCounter>(401); // construct object
+        testDst      = std::move(testSrc);                        // construct a copy
+        NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 2 && CtorDtorCounter::destroyed == 0);
         NL_TEST_ASSERT(inSuite, !testDst.IsNull() && testDst.Value().m == 401);
     }
-    NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 4 && CtorDtorCounter::destroyed == 4);
+    NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 2 && CtorDtorCounter::destroyed == 2);
 }
 
 static void TestUpdate(nlTestSuite * inSuite, void * inContext)