From 963d89a9cd1ed2e742a2022663e9875d2b104d1a Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 19 Apr 2024 13:54:10 -0400 Subject: [PATCH 01/23] make nullable depend on std::optional instead of chip::optional --- src/app/data-model/Nullable.h | 50 ++++++++++++++++++++++------------ src/app/tests/TestNullable.cpp | 4 +++ 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index cea3dea5b5361b..5f2bc8486ef25d 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -19,9 +19,9 @@ #pragma once #include -#include - +#include #include +#include namespace chip { namespace app { @@ -37,29 +37,36 @@ inline constexpr auto NullNullable = NullOptional; * things. */ template -struct Nullable : protected Optional +struct Nullable : protected std::optional { + // // The following 'using' statement is needed to make visible // all constructors of the base class within this derived class. // - using Optional::Optional; - - // Pull in APIs that make sense on Nullable with the same names as on - // Optional. - using Optional::Value; - using Optional::ValueOr; + using std::optional::optional; + using std::optional::value_or; + using std::optional::value; + using std::optional::has_value; + using std::optional::operator*; + using std::optional::operator->; + + // backwards compatibiltiyt with old chip::Optional functionality + // NOTE: as we transition to std::optional, these should be removed + T & Value() & { return value(); } + const T & Value() const & { return value(); } + Nullable(NullOptionalType) : std::optional(std::nullopt) {} // Some consumers need an easy way to determine our underlying type. using UnderlyingType = T; - constexpr void SetNull() { Optional::ClearValue(); } - constexpr bool IsNull() const { return !Optional::HasValue(); } + constexpr void SetNull() { std::optional::reset(); } + constexpr bool IsNull() const { return !std::optional::has_value(); } template constexpr T & SetNonNull(Args &&... args) { - return Optional::Emplace(std::forward(args)...); + return std::optional::emplace(std::forward(args)...); } // For integer types, being nullable involves a range restriction. @@ -96,22 +103,29 @@ struct Nullable : protected Optional // 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::operator==(other); } - bool operator!=(const Nullable & other) const { return Optional::operator!=(other); } - bool operator==(const T & other) const { return Optional::operator==(other); } - bool operator!=(const T & other) const { return Optional::operator!=(other); } + bool operator==(const Nullable & other) const + { + if (!has_value()) + { + return !other.has_value(); + } + return other.has_value() && (*other == **this); + } + bool operator!=(const Nullable & other) const { return !(*this == other); } + bool operator==(const T & other) const { return has_value() && (*this == other); } + bool operator!=(const T & other) const { return !has_value() || (*this != other); } }; template constexpr Nullable> MakeNullable(T && value) { - return Nullable>(InPlace, std::forward(value)); + return Nullable>(std::in_place, std::forward(value)); } template constexpr Nullable MakeNullable(Args &&... args) { - return Nullable(InPlace, std::forward(args)...); + return Nullable(std::in_place, std::forward(args)...); } } // namespace DataModel diff --git a/src/app/tests/TestNullable.cpp b/src/app/tests/TestNullable.cpp index c1290c1a0cef66..188b79133fbd53 100644 --- 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; From 86ab8f1973206d7b9bc5c3b433852d62f182b0d0 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 19 Apr 2024 13:59:06 -0400 Subject: [PATCH 02/23] Fix equality operator --- src/app/data-model/Nullable.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index 5f2bc8486ef25d..173dbdb55df595 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -112,7 +112,7 @@ struct Nullable : protected std::optional return other.has_value() && (*other == **this); } bool operator!=(const Nullable & other) const { return !(*this == other); } - bool operator==(const T & other) const { return has_value() && (*this == other); } + bool operator==(const T & other) const { return has_value() && (**this == other); } bool operator!=(const T & other) const { return !has_value() || (*this != other); } }; From 39c351f643ceeb07ca962ead12f130f96a5a4b92 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 19 Apr 2024 14:34:37 -0400 Subject: [PATCH 03/23] Fix inequality operator as well --- src/app/data-model/Nullable.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index 173dbdb55df595..c06275e91780e9 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -113,7 +113,7 @@ struct Nullable : protected std::optional } bool operator!=(const Nullable & other) const { return !(*this == other); } bool operator==(const T & other) const { return has_value() && (**this == other); } - bool operator!=(const T & other) const { return !has_value() || (*this != other); } + bool operator!=(const T & other) const { return !has_value() || (**this != other); } }; template From 93e5331a7bf9e3191c221d8e4f07a72c4dbf8741 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 19 Apr 2024 14:45:49 -0400 Subject: [PATCH 04/23] Fix test: previous code was enforcing chip::optional behavior on in-place construction --- src/app/tests/TestNullable.cpp | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/app/tests/TestNullable.cpp b/src/app/tests/TestNullable.cpp index 188b79133fbd53..329663779ef26a 100644 --- a/src/app/tests/TestNullable.cpp +++ b/src/app/tests/TestNullable.cpp @@ -169,24 +169,26 @@ static void TestMove(nlTestSuite * inSuite, void * inContext) CtorDtorCounter::ResetCounter(); { - auto testSrc = MakeNullable(400); - Nullable testDst(std::move(testSrc)); - NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 2 && CtorDtorCounter::destroyed == 1); + auto testSrc = MakeNullable(400); // contstruct + Nullable 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 testDst; - NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 2 && CtorDtorCounter::destroyed == 2); + Nullable testDst; // no object construction + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 0 && CtorDtorCounter::destroyed == 0); NL_TEST_ASSERT(inSuite, !!testDst.IsNull()); - auto testSrc = MakeNullable(401); - testDst = std::move(testSrc); - NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 4 && CtorDtorCounter::destroyed == 3); + auto testSrc = MakeNullable(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) From 773136da8cfb277ae8ca5458cc7f715628ac04e1 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 19 Apr 2024 14:47:02 -0400 Subject: [PATCH 05/23] Restyle --- src/app/tests/TestNullable.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/tests/TestNullable.cpp b/src/app/tests/TestNullable.cpp index 329663779ef26a..98ee07337e0ac8 100644 --- a/src/app/tests/TestNullable.cpp +++ b/src/app/tests/TestNullable.cpp @@ -179,12 +179,12 @@ static void TestMove(nlTestSuite * inSuite, void * inContext) CtorDtorCounter::ResetCounter(); { - Nullable testDst; // no object construction + Nullable testDst; // no object construction NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 0 && CtorDtorCounter::destroyed == 0); NL_TEST_ASSERT(inSuite, !!testDst.IsNull()); auto testSrc = MakeNullable(401); // construct object - testDst = std::move(testSrc); // construct a copy + 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); } From fc7b2bde9afa485cf841f957a446fa404671cb28 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 19 Apr 2024 15:10:50 -0400 Subject: [PATCH 06/23] Fix some compile errors ... std::optional now detects unused variables --- .../valve-configuration-and-control-server.cpp | 3 +-- src/app/data-model/Nullable.h | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) 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 82dd45464cfde4..9bc8001583f520 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 @@ -311,8 +311,6 @@ CHIP_ERROR SetValveLevel(EndpointId ep, DataModel::Nullable level, Data { Delegate * delegate = GetDelegate(ep); Optional status = Optional::Missing(); - DataModel::Nullable openLevel; - DataModel::Nullable autoCloseTime; CHIP_ERROR attribute_error = CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute); if (HasFeature(ep, ValveConfigurationAndControl::Feature::kTimeSync)) @@ -328,6 +326,7 @@ CHIP_ERROR SetValveLevel(EndpointId ep, DataModel::Nullable level, Data VerifyOrReturnError(UnixEpochToChipEpochMicros(utcTime.count(), chipEpochTime), CHIP_ERROR_INVALID_TIME); uint64_t time = openDuration.Value() * chip::kMicrosecondsPerSecond; + DataModel::Nullable 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 index c06275e91780e9..f729bb9b217c73 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -55,6 +55,7 @@ struct Nullable : protected std::optional // NOTE: as we transition to std::optional, these should be removed T & Value() & { return value(); } const T & Value() const & { return value(); } + const T & ValueOr(const T & defaultValue) const { return value_or(defaultValue); } Nullable(NullOptionalType) : std::optional(std::nullopt) {} // Some consumers need an easy way to determine our underlying type. From 4af74df70e14a42c085baf5eb9ee115bb4deba35 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 19 Apr 2024 15:11:13 -0400 Subject: [PATCH 07/23] Restyle --- .../valve-configuration-and-control-server.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 9bc8001583f520..36e5d02c56fbcf 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,8 +309,8 @@ CHIP_ERROR CloseValve(EndpointId ep) CHIP_ERROR SetValveLevel(EndpointId ep, DataModel::Nullable level, DataModel::Nullable openDuration) { - Delegate * delegate = GetDelegate(ep); - Optional status = Optional::Missing(); + Delegate * delegate = GetDelegate(ep); + Optional status = Optional::Missing(); CHIP_ERROR attribute_error = CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute); if (HasFeature(ep, ValveConfigurationAndControl::Feature::kTimeSync)) From 0879c6c3ef0962778f63d6d03c99b2b4f34e0b8f Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 19 Apr 2024 15:55:56 -0400 Subject: [PATCH 08/23] Remove unused variable --- .../valve-configuration-and-control-server.cpp | 1 - 1 file changed, 1 deletion(-) 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 36e5d02c56fbcf..96fa8a93e7b042 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 @@ -278,7 +278,6 @@ Delegate * GetDefaultDelegate(EndpointId endpoint) CHIP_ERROR CloseValve(EndpointId ep) { Delegate * delegate = GetDelegate(ep); - DataModel::Nullable rDuration; CHIP_ERROR attribute_error = CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute); VerifyOrReturnError(Status::Success == TargetState::Set(ep, ValveConfigurationAndControl::ValveStateEnum::kClosed), From 48635c4cdef47c535087f1402f62ee82704aa363 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 19 Apr 2024 16:01:53 -0400 Subject: [PATCH 09/23] Restyle --- .../valve-configuration-and-control-server.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 96fa8a93e7b042..02486beea315a4 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 @@ -277,7 +277,7 @@ Delegate * GetDefaultDelegate(EndpointId endpoint) CHIP_ERROR CloseValve(EndpointId ep) { - Delegate * delegate = GetDelegate(ep); + Delegate * delegate = GetDelegate(ep); CHIP_ERROR attribute_error = CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute); VerifyOrReturnError(Status::Success == TargetState::Set(ep, ValveConfigurationAndControl::ValveStateEnum::kClosed), From bc029ad733d7aaa477cfcda18ecaf74a13ad456b Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 19 Apr 2024 16:08:40 -0400 Subject: [PATCH 10/23] Review updates --- src/app/data-model/Nullable.h | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index f729bb9b217c73..356ef75df10452 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -47,15 +47,9 @@ struct Nullable : protected std::optional using std::optional::optional; using std::optional::value_or; using std::optional::value; - using std::optional::has_value; using std::optional::operator*; using std::optional::operator->; - // backwards compatibiltiyt with old chip::Optional functionality - // NOTE: as we transition to std::optional, these should be removed - T & Value() & { return value(); } - const T & Value() const & { return value(); } - const T & ValueOr(const T & defaultValue) const { return value_or(defaultValue); } Nullable(NullOptionalType) : std::optional(std::nullopt) {} // Some consumers need an easy way to determine our underlying type. @@ -106,15 +100,23 @@ struct Nullable : protected std::optional bool operator==(const Nullable & other) const { - if (!has_value()) + if (!std::optional::has_value()) { return !other.has_value(); } return other.has_value() && (*other == **this); } bool operator!=(const Nullable & other) const { return !(*this == other); } - bool operator==(const T & other) const { return has_value() && (**this == other); } - bool operator!=(const T & other) const { return !has_value() || (**this != other); } + bool operator==(const T & other) const { return std::optional::has_value() && (**this == other); } + bool operator!=(const T & other) const { return !(*this == other); } + + // backwards compatibiltiyt with old chip::Optional functionality + // NOTE: as we transition to std::optional, these should be removed + // We expect only `value` and `value_or` to remain. + T & Value() & { return value(); } + const T & Value() const & { return value(); } + const T & ValueOr(const T & defaultValue) const { return value_or(defaultValue); } + }; template From 4b0e5d95f1ecdccf9018e71a0d596c608d63eddf Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 19 Apr 2024 16:09:13 -0400 Subject: [PATCH 11/23] Review fix --- src/app/tests/TestNullable.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/tests/TestNullable.cpp b/src/app/tests/TestNullable.cpp index 98ee07337e0ac8..660037339d44e1 100644 --- a/src/app/tests/TestNullable.cpp +++ b/src/app/tests/TestNullable.cpp @@ -169,7 +169,7 @@ static void TestMove(nlTestSuite * inSuite, void * inContext) CtorDtorCounter::ResetCounter(); { - auto testSrc = MakeNullable(400); // contstruct + auto testSrc = MakeNullable(400); // construct Nullable 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); From 810758c291427b1812bd9f4f2084895ea84e72ca Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 19 Apr 2024 16:11:40 -0400 Subject: [PATCH 12/23] Typo fixes --- src/app/data-model/Nullable.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index 356ef75df10452..dfff8aa0024f4c 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -110,9 +110,11 @@ struct Nullable : protected std::optional bool operator==(const T & other) const { return std::optional::has_value() && (**this == other); } bool operator!=(const T & other) const { return !(*this == other); } - // backwards compatibiltiyt with old chip::Optional functionality + // backwards compatibility with old chip::Nullable method names + // // NOTE: as we transition to std::optional, these should be removed - // We expect only `value` and `value_or` to remain. + // We expect only `value` and `value_or` to remain as standard names + // for both nullable and optional. T & Value() & { return value(); } const T & Value() const & { return value(); } const T & ValueOr(const T & defaultValue) const { return value_or(defaultValue); } From f113924a9fbe437b0f472d54d3a0442c7735cc16 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 19 Apr 2024 16:20:32 -0400 Subject: [PATCH 13/23] Ugly forward of Value and ValueOr .... however at least hopefully it is complete --- src/app/data-model/Nullable.h | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index dfff8aa0024f4c..f04997ea5dd392 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -115,9 +115,29 @@ struct Nullable : protected std::optional // NOTE: as we transition to std::optional, these should be removed // We expect only `value` and `value_or` to remain as standard names // for both nullable and optional. - T & Value() & { return value(); } - const T & Value() const & { return value(); } - const T & ValueOr(const T & defaultValue) const { return value_or(defaultValue); } + template + auto Value(Args &&... args) -> decltype(std::optional::value(std::forward(args)...)) + { + return std::optional::value(std::forward(args)...); + } + + template + auto Value(Args &&... args) const -> decltype(std::optional::value(std::forward(args)...)) + { + return std::optional::value(std::forward(args)...); + } + + template + auto ValueOr(Args &&... args) -> decltype(std::optional::value_or(std::forward(args)...)) + { + return std::optional::value_or(std::forward(args)...); + } + + template + auto ValueOr(Args &&... args) const -> decltype(std::optional::value_or(std::forward(args)...)) + { + return std::optional::value_or(std::forward(args)...); + } }; From e0d00a0c40cabb1419b56b46acdd843d92487141 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 23 Apr 2024 09:27:37 -0400 Subject: [PATCH 14/23] Do not expose value/value_or at this time and keep Value/ValueOr as the public API for nullable --- src/app/data-model/Nullable.h | 44 ++++++++++------------------------- 1 file changed, 12 insertions(+), 32 deletions(-) diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index f04997ea5dd392..6c703f679b534a 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -44,8 +44,6 @@ struct Nullable : protected std::optional // The following 'using' statement is needed to make visible // all constructors of the base class within this derived class. // - using std::optional::optional; - using std::optional::value_or; using std::optional::value; using std::optional::operator*; using std::optional::operator->; @@ -64,6 +62,18 @@ struct Nullable : protected std::optional return std::optional::emplace(std::forward(args)...); } + template + constexpr auto ValueOr(Args &&... args) const + { + return std::optional::value_or(std::forward(args)...); + } + + template + constexpr auto Value(Args &&... args) const + { + return std::optional::value_or(std::forward(args)...); + } + // For integer types, being nullable involves a range restriction. template < typename U = std::decay_t, @@ -109,36 +119,6 @@ struct Nullable : protected std::optional bool operator!=(const Nullable & other) const { return !(*this == other); } bool operator==(const T & other) const { return std::optional::has_value() && (**this == other); } bool operator!=(const T & other) const { return !(*this == other); } - - // backwards compatibility with old chip::Nullable method names - // - // NOTE: as we transition to std::optional, these should be removed - // We expect only `value` and `value_or` to remain as standard names - // for both nullable and optional. - template - auto Value(Args &&... args) -> decltype(std::optional::value(std::forward(args)...)) - { - return std::optional::value(std::forward(args)...); - } - - template - auto Value(Args &&... args) const -> decltype(std::optional::value(std::forward(args)...)) - { - return std::optional::value(std::forward(args)...); - } - - template - auto ValueOr(Args &&... args) -> decltype(std::optional::value_or(std::forward(args)...)) - { - return std::optional::value_or(std::forward(args)...); - } - - template - auto ValueOr(Args &&... args) const -> decltype(std::optional::value_or(std::forward(args)...)) - { - return std::optional::value_or(std::forward(args)...); - } - }; template From aef95c103d2ffadd0ae267a9edb1fd0a87daaac8 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 23 Apr 2024 09:48:42 -0400 Subject: [PATCH 15/23] Fix copy and paste typo --- src/app/data-model/Nullable.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index 6c703f679b534a..248add09c2a152 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -71,7 +71,7 @@ struct Nullable : protected std::optional template constexpr auto Value(Args &&... args) const { - return std::optional::value_or(std::forward(args)...); + return std::optional::value(std::forward(args)...); } // For integer types, being nullable involves a range restriction. From 589cf1220eda8cbdc87992f817e9d358e8364787 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 23 Apr 2024 09:56:08 -0400 Subject: [PATCH 16/23] Make more changes to make things compile and work like the old nullable --- src/app/data-model/Nullable.h | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index 248add09c2a152..6490daff68e4a4 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -44,7 +44,7 @@ struct Nullable : protected std::optional // The following 'using' statement is needed to make visible // all constructors of the base class within this derived class. // - using std::optional::value; + using std::optional::optional; using std::optional::operator*; using std::optional::operator->; @@ -68,11 +68,8 @@ struct Nullable : protected std::optional return std::optional::value_or(std::forward(args)...); } - template - constexpr auto Value(Args &&... args) const - { - return std::optional::value(std::forward(args)...); - } + constexpr const T & Value() const { return **this; } + T & Value() { return **this; } // For integer types, being nullable involves a range restriction. template < From 569f7cae938dfe0fa25ef87d963f2b00bc209517 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 23 Apr 2024 10:59:44 -0400 Subject: [PATCH 17/23] Use value again instead of operator** ... I was looking at the wrong compiler errors --- src/app/data-model/Nullable.h | 10 +++++----- src/app/server/CommissioningWindowManager.cpp | 6 ++++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index 6490daff68e4a4..f70e483f5c6fdf 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -62,14 +62,14 @@ struct Nullable : protected std::optional return std::optional::emplace(std::forward(args)...); } - template - constexpr auto ValueOr(Args &&... args) const + template + constexpr auto ValueOr(Arg && arg) const { - return std::optional::value_or(std::forward(args)...); + return std::optional::value_or(std::forward(arg)); } - constexpr const T & Value() const { return **this; } - T & Value() { return **this; } + inline constexpr const T & Value() const { return std::optional::value(); } + inline T & Value() { return std::optional::value(); } // For integer types, being nullable involves a range restriction. template < diff --git a/src/app/server/CommissioningWindowManager.cpp b/src/app/server/CommissioningWindowManager.cpp index f11261ad9c444d..80a2b217aaf2f4 100644 --- a/src/app/server/CommissioningWindowManager.cpp +++ b/src/app/server/CommissioningWindowManager.cpp @@ -40,6 +40,8 @@ using chip::app::DataModel::NullNullable; namespace { +constexpr uint32_t kFailSafeTimeoutSeconds = 3; + // As per specifications (Section 13.3), Nodes SHALL exit commissioning mode after 20 failed commission attempts. constexpr uint8_t kMaxFailedCommissioningAttempts = 20; @@ -174,7 +176,7 @@ void CommissioningWindowManager::HandleFailedAttempt(CHIP_ERROR err) void CommissioningWindowManager::OnSessionEstablishmentStarted() { // As per specifications, section 5.5: Commissioning Flows - constexpr System::Clock::Timeout kPASESessionEstablishmentTimeout = System::Clock::Seconds16(60); + constexpr System::Clock::Timeout kPASESessionEstablishmentTimeout = System::Clock::Seconds16(kFailSafeTimeoutSeconds); DeviceLayer::SystemLayer().StartTimer(kPASESessionEstablishmentTimeout, HandleSessionEstablishmentTimeout, this); ChipLogProgress(AppServer, "Commissioning session establishment step started"); @@ -208,7 +210,7 @@ void CommissioningWindowManager::OnSessionEstablished(const SessionHandle & sess } else { - err = failSafeContext.ArmFailSafe(kUndefinedFabricIndex, System::Clock::Seconds16(60)); + err = failSafeContext.ArmFailSafe(kUndefinedFabricIndex, System::Clock::Seconds16(kFailSafeTimeoutSeconds)); if (err != CHIP_NO_ERROR) { ChipLogError(AppServer, "Error arming failsafe on PASE session establishment completion"); From 736d9e60da53fb2dedea08d89a4886578a02fb79 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 23 Apr 2024 11:03:20 -0400 Subject: [PATCH 18/23] Undo unrelated change --- src/app/server/CommissioningWindowManager.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/app/server/CommissioningWindowManager.cpp b/src/app/server/CommissioningWindowManager.cpp index 80a2b217aaf2f4..f11261ad9c444d 100644 --- a/src/app/server/CommissioningWindowManager.cpp +++ b/src/app/server/CommissioningWindowManager.cpp @@ -40,8 +40,6 @@ using chip::app::DataModel::NullNullable; namespace { -constexpr uint32_t kFailSafeTimeoutSeconds = 3; - // As per specifications (Section 13.3), Nodes SHALL exit commissioning mode after 20 failed commission attempts. constexpr uint8_t kMaxFailedCommissioningAttempts = 20; @@ -176,7 +174,7 @@ void CommissioningWindowManager::HandleFailedAttempt(CHIP_ERROR err) void CommissioningWindowManager::OnSessionEstablishmentStarted() { // As per specifications, section 5.5: Commissioning Flows - constexpr System::Clock::Timeout kPASESessionEstablishmentTimeout = System::Clock::Seconds16(kFailSafeTimeoutSeconds); + constexpr System::Clock::Timeout kPASESessionEstablishmentTimeout = System::Clock::Seconds16(60); DeviceLayer::SystemLayer().StartTimer(kPASESessionEstablishmentTimeout, HandleSessionEstablishmentTimeout, this); ChipLogProgress(AppServer, "Commissioning session establishment step started"); @@ -210,7 +208,7 @@ void CommissioningWindowManager::OnSessionEstablished(const SessionHandle & sess } else { - err = failSafeContext.ArmFailSafe(kUndefinedFabricIndex, System::Clock::Seconds16(kFailSafeTimeoutSeconds)); + err = failSafeContext.ArmFailSafe(kUndefinedFabricIndex, System::Clock::Seconds16(60)); if (err != CHIP_NO_ERROR) { ChipLogError(AppServer, "Error arming failsafe on PASE session establishment completion"); From b3cc106bebeb9fa27a03e9698ef44b99e7e50d73 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 23 Apr 2024 13:22:35 -0400 Subject: [PATCH 19/23] Make clang-tidy be abel to compile ... apparently it really does not like using has_value in equality operators --- .../ota-requestor/ota-requestor-server.cpp | 1 - src/app/data-model/Nullable.h | 15 +++++++-------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/app/clusters/ota-requestor/ota-requestor-server.cpp b/src/app/clusters/ota-requestor/ota-requestor-server.cpp index 7bfbde1fa54ac5..bab7941df7b488 100644 --- a/src/app/clusters/ota-requestor/ota-requestor-server.cpp +++ b/src/app/clusters/ota-requestor/ota-requestor-server.cpp @@ -182,7 +182,6 @@ Status OtaRequestorServerGetUpdateState(chip::EndpointId endpointId, OTAUpdateSt Status OtaRequestorServerSetUpdateStateProgress(app::DataModel::Nullable value) { Status status = Status::Success; - // Find all endpoints that have OtaSoftwareUpdateRequestor implemented for (auto endpoint : EnabledEndpointsWithServerCluster(OtaSoftwareUpdateRequestor::Id)) { diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index f70e483f5c6fdf..2eaeb5258bfb21 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -105,17 +105,16 @@ struct Nullable : protected std::optional // 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 + bool operator==(const T & other) const { return std::optional::has_value() && (**this == other); } + bool operator!=(const T & other) const { return !(*this == other); } + + inline bool operator==(const Nullable & other) const { - if (!std::optional::has_value()) - { - return !other.has_value(); - } - return other.has_value() && (*other == **this); + return static_cast&>(*this) == + static_cast&>(other); } bool operator!=(const Nullable & other) const { return !(*this == other); } - bool operator==(const T & other) const { return std::optional::has_value() && (**this == other); } - bool operator!=(const T & other) const { return !(*this == other); } + }; template From f8d8b3b34833c38a7de89d7b75cc5760f5968c3a Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 23 Apr 2024 13:52:04 -0400 Subject: [PATCH 20/23] Restyle --- src/app/data-model/Nullable.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index 2eaeb5258bfb21..7136d49b76e7ef 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -110,11 +110,9 @@ struct Nullable : protected std::optional inline bool operator==(const Nullable & other) const { - return static_cast&>(*this) == - static_cast&>(other); + return static_cast &>(*this) == static_cast &>(other); } bool operator!=(const Nullable & other) const { return !(*this == other); } - }; template From 4796bf6dada3999d087174550741459c5f9d9f47 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 23 Apr 2024 15:34:57 -0400 Subject: [PATCH 21/23] Another update to make clang-tidy happy for the other equality operator --- src/app/data-model/Nullable.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index 7136d49b76e7ef..77f3ab68301ab1 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -105,14 +105,14 @@ struct Nullable : protected std::optional // 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 T & other) const { return std::optional::has_value() && (**this == other); } - bool operator!=(const T & other) const { return !(*this == other); } + inline bool operator==(const T & other) const { return static_cast &>(*this) == other; } + inline bool operator!=(const T & other) const { return !(*this == other); } inline bool operator==(const Nullable & other) const { return static_cast &>(*this) == static_cast &>(other); } - bool operator!=(const Nullable & other) const { return !(*this == other); } + inline bool operator!=(const Nullable & other) const { return !(*this == other); } }; template From 9bb289e6844b557ebe36f2e6e121f1e08950006b Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 29 Apr 2024 11:28:48 -0400 Subject: [PATCH 22/23] Update src/app/data-model/Nullable.h Co-authored-by: Boris Zbarsky --- src/app/data-model/Nullable.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index 77f3ab68301ab1..f56e67ced19113 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -62,10 +62,10 @@ struct Nullable : protected std::optional return std::optional::emplace(std::forward(args)...); } - template - constexpr auto ValueOr(Arg && arg) const + template + constexpr auto ValueOr(Args &&... args) const { - return std::optional::value_or(std::forward(arg)); + return std::optional::value_or(std::forward(args)...); } inline constexpr const T & Value() const { return std::optional::value(); } From 25055dd179ce2278922269925d7d0fd1f06d3c70 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 29 Apr 2024 11:30:46 -0400 Subject: [PATCH 23/23] Undo odd whitespace change --- src/app/clusters/ota-requestor/ota-requestor-server.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/app/clusters/ota-requestor/ota-requestor-server.cpp b/src/app/clusters/ota-requestor/ota-requestor-server.cpp index bab7941df7b488..7bfbde1fa54ac5 100644 --- a/src/app/clusters/ota-requestor/ota-requestor-server.cpp +++ b/src/app/clusters/ota-requestor/ota-requestor-server.cpp @@ -182,6 +182,7 @@ Status OtaRequestorServerGetUpdateState(chip::EndpointId endpointId, OTAUpdateSt Status OtaRequestorServerSetUpdateStateProgress(app::DataModel::Nullable value) { Status status = Status::Success; + // Find all endpoints that have OtaSoftwareUpdateRequestor implemented for (auto endpoint : EnabledEndpointsWithServerCluster(OtaSoftwareUpdateRequestor::Id)) {