Skip to content

Commit 4393200

Browse files
tcarmelveilleuxrestyled-commitsandy31415
authored
Add tests to lib/support/SortUtils.h (project-chip#32421)
* Add tests to lib/support/SortUtils.h - SortUtils.h has no tests. While it is apparently trivial, we should never have code that has no tests - BubbleSort could crash (never-ending loop due to integer overflow) when input size was < 2. Its use is removed and replaced by InsertionSort which is known to be faster on average. A better solution is to move to using STL sorts in the future, but this requires analysis of flash costs and STL usage, given lambdas are used with sorts many places. Fixes project-chip#32420 Testing done: - Added missing unit tests. - All integration tests still pass. * Restyled by clang-format * Fix build error on Android * Add constness to operator== --------- Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: Andrei Litvin <andy314@gmail.com>
1 parent 92d87e8 commit 4393200

File tree

6 files changed

+178
-49
lines changed

6 files changed

+178
-49
lines changed

src/app/clusters/time-synchronization-server/time-synchronization-server.cpp

-1
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
#include <app/server/Server.h>
3030
#include <app/util/attribute-storage.h>
3131
#include <lib/support/CodeUtils.h>
32-
#include <lib/support/SortUtils.h>
3332
#include <lib/support/logging/CHIPLogging.h>
3433
#include <platform/CHIPDeviceLayer.h>
3534
#include <platform/RuntimeOptionsProvider.h>

src/lib/dnssd/IPAddressSorter.cpp

+7-7
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ namespace IPAddressSorter {
2424

2525
void Sort(Inet::IPAddress * addresses, size_t count, Inet::InterfaceId interfaceId)
2626
{
27-
Sorting::BubbleSort(addresses, count, [interfaceId](const Inet::IPAddress & a, const Inet::IPAddress & b) -> bool {
27+
Sorting::InsertionSort(addresses, count, [interfaceId](const Inet::IPAddress & a, const Inet::IPAddress & b) -> bool {
2828
auto scoreA = to_underlying(ScoreIpAddress(a, interfaceId));
2929
auto scoreB = to_underlying(ScoreIpAddress(b, interfaceId));
3030
return scoreA > scoreB;
@@ -33,12 +33,12 @@ void Sort(Inet::IPAddress * addresses, size_t count, Inet::InterfaceId interface
3333

3434
void Sort(const Span<Inet::IPAddress> & addresses, Inet::InterfaceId interfaceId)
3535
{
36-
Sorting::BubbleSort(addresses.begin(), addresses.size(),
37-
[interfaceId](const Inet::IPAddress & a, const Inet::IPAddress & b) -> bool {
38-
auto scoreA = to_underlying(ScoreIpAddress(a, interfaceId));
39-
auto scoreB = to_underlying(ScoreIpAddress(b, interfaceId));
40-
return scoreA > scoreB;
41-
});
36+
Sorting::InsertionSort(addresses.data(), addresses.size(),
37+
[interfaceId](const Inet::IPAddress & a, const Inet::IPAddress & b) -> bool {
38+
auto scoreA = to_underlying(ScoreIpAddress(a, interfaceId));
39+
auto scoreB = to_underlying(ScoreIpAddress(b, interfaceId));
40+
return scoreA > scoreB;
41+
});
4242
}
4343

4444
IpScore ScoreIpAddress(const Inet::IPAddress & ip, Inet::InterfaceId interfaceId)

src/lib/support/SortUtils.h

-40
Original file line numberDiff line numberDiff line change
@@ -26,46 +26,6 @@
2626
namespace chip {
2727
namespace Sorting {
2828

29-
/**
30-
*
31-
* Impements the bubble sort algorithm to sort an array
32-
* of items of size 'n'.
33-
*
34-
* T should be swappable using std::swap (i.e implements Swappable).
35-
*
36-
* The provided comparison function SHALL have the following signature:
37-
*
38-
* bool Compare(const T& a, const T& b);
39-
*
40-
* If a is deemed less than (i.e is ordered before) b, return true.
41-
* Else, return false.
42-
*
43-
* This is a stable sort.
44-
*
45-
* NOTE: This has a worst case time complexity of O(n^2). This
46-
* is well-suited for small arrays where the time trade-off is
47-
* acceptable compared to the lower flash cost for the simple sorting
48-
* implementation.
49-
*
50-
*/
51-
template <typename T, typename CompareFunc>
52-
void BubbleSort(T * items, size_t n, CompareFunc f)
53-
{
54-
for (size_t i = 0; i < (n - 1); i++)
55-
{
56-
for (size_t j = 0; j < (n - i - 1); j++)
57-
{
58-
const T & a = items[j + 1];
59-
const T & b = items[j];
60-
61-
if (f(a, b))
62-
{
63-
std::swap(items[j], items[j + 1]);
64-
}
65-
}
66-
}
67-
}
68-
6929
/**
7030
*
7131
* Impements the insertion sort algorithm to sort an array

src/lib/support/tests/BUILD.gn

+1
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ chip_test_suite_using_nltest("tests") {
4545
"TestSafeString.cpp",
4646
"TestScoped.cpp",
4747
"TestScopedBuffer.cpp",
48+
"TestSorting.cpp",
4849
"TestSpan.cpp",
4950
"TestStateMachine.cpp",
5051
"TestStaticSupportSmartPtr.cpp",

src/lib/support/tests/TestSorting.cpp

+169
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
/*
2+
*
3+
* Copyright (c) 2024 Project CHIP Authors
4+
* All rights reserved.
5+
*
6+
* Licensed under the Apache License, Version 2.0 (the "License");
7+
* you may not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
19+
#include <algorithm>
20+
#include <array>
21+
#include <stdio.h>
22+
23+
#include <lib/support/SortUtils.h>
24+
#include <lib/support/Span.h>
25+
26+
#include <lib/support/UnitTestRegistration.h>
27+
#include <nlunit-test.h>
28+
29+
using namespace chip;
30+
using namespace chip::Sorting;
31+
32+
namespace {
33+
34+
struct Datum
35+
{
36+
// Key is the element that takes part in sorting.
37+
int key;
38+
// The associated data is constructed to detect stable sort.
39+
int associated_data;
40+
41+
bool operator==(const Datum & other) const
42+
{
43+
return (this == &other) || ((this->key == other.key) && (this->associated_data == other.associated_data));
44+
}
45+
};
46+
47+
class Sorter
48+
{
49+
public:
50+
virtual ~Sorter() = default;
51+
52+
void Reset() { m_compare_count = 0; }
53+
54+
size_t compare_count() const { return m_compare_count; }
55+
56+
virtual void Sort(chip::Span<Datum> data) = 0;
57+
58+
protected:
59+
size_t m_compare_count = 0;
60+
};
61+
62+
class InsertionSorter : public Sorter
63+
{
64+
public:
65+
void Sort(chip::Span<Datum> data)
66+
{
67+
InsertionSort(data.data(), data.size(), [&](const Datum & a, const Datum & b) -> bool {
68+
++m_compare_count;
69+
return a.key < b.key;
70+
});
71+
}
72+
};
73+
74+
void DoBasicSortTest(nlTestSuite * inSuite, Sorter & sorter)
75+
{
76+
Span<Datum> empty_to_sort;
77+
Span<Datum> empty_expected;
78+
79+
std::array<Datum, 1> single_entry_to_sort{ { { 1, 100 } } };
80+
std::array<Datum, 1> single_entry_expected{ { { 1, 100 } } };
81+
82+
std::array<Datum, 2> two_entries_to_sort{ { { 2, 200 }, { 1, 100 } } };
83+
std::array<Datum, 2> two_entries_expected{ { { 1, 100 }, { 2, 200 } } };
84+
85+
std::array<Datum, 20> random_order_to_sort{ { { 1, 100 }, { 10, 1000 }, { 2, 200 }, { 13, 1300 }, { 4, 400 },
86+
{ 8, 800 }, { 10, 1001 }, { 6, 600 }, { 7, 700 }, { 7, 701 },
87+
{ 6, 601 }, { 6, 602 }, { 6, 603 }, { 8, 801 }, { 14, 1400 },
88+
{ 6, 604 }, { 10, 1002 }, { 12, 1200 }, { 4, 401 }, { 2, 201 } } };
89+
90+
std::array<Datum, 20> random_order_expected{ { { 1, 100 }, { 2, 200 }, { 2, 201 }, { 4, 400 }, { 4, 401 },
91+
{ 6, 600 }, { 6, 601 }, { 6, 602 }, { 6, 603 }, { 6, 604 },
92+
{ 7, 700 }, { 7, 701 }, { 8, 800 }, { 8, 801 }, { 10, 1000 },
93+
{ 10, 1001 }, { 10, 1002 }, { 12, 1200 }, { 13, 1300 }, { 14, 1400 } } };
94+
95+
std::array<Datum, 20> reverse_order_to_sort{ { { 20, 2000 }, { 19, 1900 }, { 18, 1800 }, { 17, 1700 }, { 16, 1600 },
96+
{ 15, 1500 }, { 14, 1400 }, { 13, 1300 }, { 12, 1200 }, { 11, 1100 },
97+
{ 10, 1000 }, { 9, 900 }, { 8, 800 }, { 7, 700 }, { 6, 600 },
98+
{ 5, 500 }, { 4, 400 }, { 3, 300 }, { 2, 200 }, { 1, 100 } } };
99+
100+
std::array<Datum, 20> reverse_order_expected{ { { 1, 100 }, { 2, 200 }, { 3, 300 }, { 4, 400 }, { 5, 500 },
101+
{ 6, 600 }, { 7, 700 }, { 8, 800 }, { 9, 900 }, { 10, 1000 },
102+
{ 11, 1100 }, { 12, 1200 }, { 13, 1300 }, { 14, 1400 }, { 15, 1500 },
103+
{ 16, 1600 }, { 17, 1700 }, { 18, 1800 }, { 19, 1900 }, { 20, 2000 } } };
104+
105+
std::array<Span<Datum>, 5> inputs = { { empty_to_sort, Span<Datum>(single_entry_to_sort), Span<Datum>(two_entries_to_sort),
106+
Span<Datum>(random_order_to_sort), Span<Datum>(reverse_order_to_sort) } };
107+
std::array<Span<Datum>, 5> expected_outs = { { empty_expected, Span<Datum>(single_entry_expected),
108+
Span<Datum>(two_entries_expected), Span<Datum>(random_order_expected),
109+
Span<Datum>(reverse_order_expected) } };
110+
111+
for (size_t case_idx = 0; case_idx < inputs.size(); ++case_idx)
112+
{
113+
printf("Case index: %d\n", static_cast<int>(case_idx));
114+
auto & to_sort = inputs[case_idx];
115+
const auto & expected = expected_outs[case_idx];
116+
117+
sorter.Sort(to_sort);
118+
NL_TEST_ASSERT(inSuite, to_sort.data_equal(expected));
119+
if (!to_sort.data_equal(expected))
120+
{
121+
for (size_t idx = 0; idx < to_sort.size(); ++idx)
122+
{
123+
Datum sorted_item = to_sort[idx];
124+
Datum expected_item = expected[idx];
125+
printf("Index: %d, got { %d, %d }, expected { %d, %d }\n", static_cast<int>(idx), sorted_item.key,
126+
sorted_item.associated_data, expected_item.key, expected_item.associated_data);
127+
}
128+
}
129+
NL_TEST_ASSERT(inSuite, sorter.compare_count() <= (to_sort.size() * to_sort.size()));
130+
printf("Compare counts: %d\n", static_cast<int>(sorter.compare_count()));
131+
sorter.Reset();
132+
}
133+
}
134+
135+
void TestBasicSort(nlTestSuite * inSuite, void * inContext)
136+
{
137+
printf("Testing insertion sorter.\n");
138+
InsertionSorter insertion_sorter;
139+
DoBasicSortTest(inSuite, insertion_sorter);
140+
}
141+
142+
// clang-format off
143+
static const nlTest sTests[] =
144+
{
145+
NL_TEST_DEF("Basic sort tests for custom sort utilities", TestBasicSort),
146+
NL_TEST_SENTINEL()
147+
};
148+
// clang-format on
149+
150+
} // namespace
151+
152+
int TestSortUtils()
153+
{
154+
// clang-format off
155+
nlTestSuite theSuite =
156+
{
157+
"Test for SortUtils",
158+
&sTests[0],
159+
nullptr,
160+
nullptr
161+
};
162+
// clang-format on
163+
164+
nlTestRunner(&theSuite, nullptr);
165+
166+
return (nlTestRunnerStats(&theSuite));
167+
}
168+
169+
CHIP_REGISTER_TEST_SUITE(TestSortUtils)

src/transport/SecureSessionTable.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ class SecureSessionTable
183183
template <typename CompareFunc>
184184
void Sort(CompareFunc func)
185185
{
186-
Sorting::BubbleSort(mSessionList.begin(), mSessionList.size(), func);
186+
Sorting::InsertionSort(mSessionList.begin(), mSessionList.size(), func);
187187
}
188188

189189
const ScopedNodeId & GetSessionEvictionHint() const { return mSessionEvictionHint; }

0 commit comments

Comments
 (0)