Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify and future-proof enum range checks in door locks. #24879

Merged
merged 3 commits into from
Feb 8, 2023
Merged

Simplify and future-proof enum range checks in door locks. #24879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ff5e74
Simplify and future-proof enum range checks in door locks.
bzbarsky-apple Feb 6, 2023
d8b2134
Address review commment.
bzbarsky-apple Feb 6, 2023
2d8aa7d
Address more review comments.
bzbarsky-apple Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions src/app/clusters/door-lock-server/door-lock-server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,11 +380,10 @@ void DoorLockServer::setUserCommandHandler(chip::app::CommandHandler * commandOb
return;
}

if (!userType.IsNull() &&
(userType.Value() < UserTypeEnum::kUnrestrictedUser || userType.Value() > UserTypeEnum::kRemoteOnlyUser))
if (userType == UserTypeEnum::kUnknownEnumValue)
{
emberAfDoorLockClusterPrintln(
"[SetUser] Unable to set the user: user type is out of range [endpointId=%d,userIndex=%d,userType=%u]",
"[SetUser] Unable to set the user: user type is unknown [endpointId=%d,userIndex=%d,userType=%u]",
commandPath.mEndpointId, userIndex, to_underlying(userType.Value()));

sendClusterResponse(commandObj, commandPath, EMBER_ZCL_STATUS_INVALID_COMMAND);
Expand Down Expand Up @@ -694,10 +693,9 @@ void DoorLockServer::setCredentialCommandHandler(
return;
}

if (!userType.IsNull() &&
(userType.Value() < UserTypeEnum::kUnrestrictedUser || userType.Value() > UserTypeEnum::kRemoteOnlyUser))
if (userType == UserTypeEnum::kUnknownEnumValue)
{
emberAfDoorLockClusterPrintln("[SetCredential] Unable to set the credential: user type is out of range "
emberAfDoorLockClusterPrintln("[SetCredential] Unable to set the credential: user type is unknown "
"[endpointId=%d,credentialIndex=%d,userType=%u]",
commandPath.mEndpointId, credentialIndex, to_underlying(userType.Value()));
sendSetCredentialResponse(commandObj, commandPath, DlStatus::kInvalidField, 0, nextAvailableCredentialSlot);
Expand Down Expand Up @@ -3113,9 +3111,9 @@ void DoorLockServer::setHolidayScheduleCommandHandler(chip::app::CommandHandler
return;
}

if (operatingMode > OperatingModeEnum::kPassage || operatingMode < OperatingModeEnum::kNormal)
if (operatingMode == OperatingModeEnum::kUnknownEnumValue)
{
emberAfDoorLockClusterPrintln("[SetHolidaySchedule] Unable to add schedule - operating mode is out of range"
emberAfDoorLockClusterPrintln("[SetHolidaySchedule] Unable to add schedule - operating mode is unknown"
"[endpointId=%d,scheduleIndex=%d,localStarTime=%" PRIu32 ",localEndTime=%" PRIu32
", operatingMode=%d]",
endpointId, holidayIndex, localStartTime, localEndTime, to_underlying(operatingMode));
Expand Down
4 changes: 3 additions & 1 deletion src/app/data-model/Nullable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,9 @@ struct Nullable : protected Optional<T>
static constexpr bool kIsFabricScoped = false;

bool operator==(const Nullable & other) const { return Optional<T>::operator==(other); }
bool operator!=(const Nullable & other) const { return !(*this == 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); }
};

template <class T>
Expand Down
2 changes: 2 additions & 0 deletions src/lib/core/Optional.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,8 @@ class Optional
return (mHasValue == other.mHasValue) && (!other.mHasValue || (mValue.mData == other.mValue.mData));
}
bool operator!=(const Optional & other) const { return !(*this == other); }
bool operator==(const T & other) const { return HasValue() && Value() == other; }
bool operator!=(const T & other) const { return !(*this == other); }

/** Convenience method to create an optional without a valid value. */
static Optional<T> Missing() { return Optional<T>(); }
Expand Down
19 changes: 19 additions & 0 deletions src/lib/core/tests/TestOptional.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ struct Count
Count(Count && o) : m(o.m) { ++created; }
Count & operator=(Count &&) = default;

bool operator==(const Count & o) const { return m == o.m; }

int m;

static void ResetCounter()
Expand Down Expand Up @@ -74,26 +76,43 @@ int Count::destroyed;

static void TestBasic(nlTestSuite * inSuite, void * inContext)
{
// Set up our test Count objects, which will mess with counts, before we reset the
// counts.
Count c100(100), c101(101), c102(102);

Count::ResetCounter();

{
auto testOptional = Optional<Count>::Value(100);
NL_TEST_ASSERT(inSuite, Count::created == 1 && Count::destroyed == 0);
NL_TEST_ASSERT(inSuite, testOptional.HasValue() && testOptional.Value().m == 100);
NL_TEST_ASSERT(inSuite, testOptional == c100);
NL_TEST_ASSERT(inSuite, testOptional != c101);
NL_TEST_ASSERT(inSuite, testOptional != c102);

testOptional.ClearValue();
NL_TEST_ASSERT(inSuite, Count::created == 1 && Count::destroyed == 1);
NL_TEST_ASSERT(inSuite, !testOptional.HasValue());
NL_TEST_ASSERT(inSuite, testOptional != c100);
NL_TEST_ASSERT(inSuite, testOptional != c101);
NL_TEST_ASSERT(inSuite, testOptional != c102);

testOptional.SetValue(Count(101));
NL_TEST_ASSERT(inSuite, Count::created == 3 && Count::destroyed == 2);
NL_TEST_ASSERT(inSuite, testOptional.HasValue() && testOptional.Value().m == 101);
NL_TEST_ASSERT(inSuite, testOptional != c100);
NL_TEST_ASSERT(inSuite, testOptional == c101);
NL_TEST_ASSERT(inSuite, testOptional != c102);

testOptional.Emplace(102);
NL_TEST_ASSERT(inSuite, Count::created == 4 && Count::destroyed == 3);
NL_TEST_ASSERT(inSuite, testOptional.HasValue() && testOptional.Value().m == 102);
NL_TEST_ASSERT(inSuite, testOptional != c100);
NL_TEST_ASSERT(inSuite, testOptional != c101);
NL_TEST_ASSERT(inSuite, testOptional == c102);
}

// Our test Count objects are still in scope here.
NL_TEST_ASSERT(inSuite, Count::created == 4 && Count::destroyed == 4);
}

Expand Down