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

.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});