From dc2d83a3f19dd247713d2974ff10b06cfa3c3ce4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 13 Jun 2026 15:52:07 +0000 Subject: [PATCH 1/2] Fix set::min()/max() comparator bug and non-const operator[] (#28) set::min()/max() used std::min_element/std::max_element, which rank by operator< instead of the set's TCompare. For sets with a custom comparator this returned the wrong element, required operator< even when a comparator was supplied, and was O(n). They now return *begin() / *std::prev(end()), which respects the comparator and is O(1). The non-const set::operator[] did `auto it = std::advance(begin(), index)`, but std::advance returns void, so the overload failed to compile whenever instantiated (subscripting a non-const set). It now advances an iterator in place, matching the const overload, and works on both C++11 and C++17. Added tests: min/max with a comparator that differs from operator< (descending int comparator), and non-const subscripting. --- include/set.h | 30 +++++++++++++----------------- tests/set_test.cc | 29 +++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 17 deletions(-) diff --git a/include/set.h b/include/set.h index 58f6ac7..b0a5e35 100644 --- a/include/set.h +++ b/include/set.h @@ -606,11 +606,13 @@ namespace fcpp { // minimum.value() -> 1 [[nodiscard]] fcpp::optional_t min() const { - const auto& it = std::min_element(begin(), end()); - if (it != end()) { - return *it; + if (m_set.empty()) { + return fcpp::optional_t(); } - return fcpp::optional_t(); + // The set is already ordered by TCompare, so the minimum is the first + // element (O(1)). Using std::min_element would re-rank by operator<, + // ignoring the custom comparator and costing O(n). + return *begin(); } // Returns the maximum key in the set, if it's not empty. @@ -627,11 +629,13 @@ namespace fcpp { // maximum.value() -> 8 [[nodiscard]] fcpp::optional_t max() const { - const auto& it = std::max_element(begin(), end()); - if (it != end()) { - return *it; + if (m_set.empty()) { + return fcpp::optional_t(); } - return fcpp::optional_t(); + // The set is already ordered by TCompare, so the maximum is the last + // element (O(1)). Using std::max_element would re-rank by operator<, + // ignoring the custom comparator and costing O(n). + return *std::prev(end()); } // Performs the functional `map` algorithm, in which every element of the resulting set is the @@ -1102,17 +1106,9 @@ namespace fcpp { TKey operator[](size_t index) { assert_smaller_size(index); -#ifdef CPP17_AVAILABLE - auto it = std::advance(begin(), index); - return *it; -#else - auto count = 0; auto it = begin(); - while (count++ < index) { - ++it; - } + std::advance(it, index); return *it; -#endif } // Returns a copy of the key at the given sorted position. diff --git a/tests/set_test.cc b/tests/set_test.cc index 6751d9d..3a83a3f 100644 --- a/tests/set_test.cc +++ b/tests/set_test.cc @@ -332,6 +332,35 @@ TEST(SetTest, MaxEmptySet) EXPECT_FALSE(maximum.has_value()); } +// min()/max() must respect the set's own comparator, not operator<. +// The descending comparator orders ints opposite to operator<, so the +// comparator-smallest element is the largest int, and vice versa. +TEST(SetTest, MinRespectsCustomComparator) +{ + const set numbers( + make_stateful_descending_set({1, 4, 2, 5, 8, 3})); + const auto minimum = numbers.min(); + EXPECT_TRUE(minimum.has_value()); + EXPECT_EQ(8, minimum.value()); +} + +TEST(SetTest, MaxRespectsCustomComparator) +{ + const set numbers( + make_stateful_descending_set({1, 4, 2, 5, 8, 3})); + const auto maximum = numbers.max(); + EXPECT_TRUE(maximum.has_value()); + EXPECT_EQ(1, maximum.value()); +} + +TEST(SetTest, NonConstSubscripting) +{ + set set_under_test(std::set({1, 5, 3, 3})); + EXPECT_EQ(1, set_under_test[0]); + EXPECT_EQ(3, set_under_test[1]); + EXPECT_EQ(5, set_under_test[2]); +} + TEST(SetTest, Map) { const set numbers({4, 1, 3}); From 8a0e78a407407d2a6f8e180ed40804685bd0ad61 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 13 Jun 2026 15:57:50 +0000 Subject: [PATCH 2/2] CI: pin Windows runner to windows-2022 for VS 2022 generator GitHub's windows-latest image no longer provides the "Visual Studio 17 2022" generator, so CMake configure failed with "could not find any instance of Visual Studio" before compiling. Pin the Windows matrix entries to windows-2022. --- .github/workflows/cmake.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/cmake.yml b/.github/workflows/cmake.yml index 7a6b0ef..dfdc595 100644 --- a/.github/workflows/cmake.yml +++ b/.github/workflows/cmake.yml @@ -22,7 +22,7 @@ jobs: config: - { name: "Windows Latest MSVC (C++11)", - os: windows-latest, + os: windows-2022, build_type: "Debug", cc: "cl", cxx: "cl", @@ -31,7 +31,7 @@ jobs: } - { name: "Windows Latest MSVC (C++17)", - os: windows-latest, + os: windows-2022, build_type: "Debug", cc: "cl", cxx: "cl",