Fix set::min()/max() comparator bug and non-const operator[] (#32)

* 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.

* 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.

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: jkalias

.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",

include/set.h

@@ -606,11 +606,13 @@ namespace fcpp {
         //      minimum.value() -> 1
         [[nodiscard]] fcpp::optional_t<TKey> min() const
         {
-            const auto& it = std::min_element(begin(), end());
-            if (it != end()) {
-                return *it;
+            if (m_set.empty()) {
+                return fcpp::optional_t<TKey>();
             }
-            return fcpp::optional_t<TKey>();
+            // 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<TKey> max() const
         {
-            const auto& it = std::max_element(begin(), end());
-            if (it != end()) {
-                return *it;
+            if (m_set.empty()) {
+                return fcpp::optional_t<TKey>();
             }
-            return fcpp::optional_t<TKey>();
+            // 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.

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<int, stateful_descending_int_compare> 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<int, stateful_descending_int_compare> 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<int> set_under_test(std::set<int>({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<int> numbers({4, 1, 3});