[libcxx-commits] [PATCH] D119589: [libc++][ranges] Implement ranges::min

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

The code looks good, but it seems we lack a bit of test coverage.



================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:92
+    assert(ret.i == 1);
+  }
+
----------------
I miss tests to compare the Complexity `Complexity: Exactly one comparison and two applications of the projection, if any.`



================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:93
+  }
+
+  { // check that pointers are compared and not a range
----------------
I miss a test for two equivalent values. `Returns: The smaller value. Returns the first argument when the arguments are equivalent.`
We need to test the first argument is returned.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:147
+    assert(ret == 1);
+  }
+}
----------------
Please add a test with multiple equivalent values:
* The first element has multiple equivalent values
* Another element has multiple equivalent values
That way we make sure we validate `Returns: The smallest value in the input range. Returns a copy of the leftmost element when several elements are equivalent to the smallest.`

The same for `test_range`


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:150
+
+constexpr void test_range() {
+  int a[] = {7, 6, 9, 3, 5, 1, 2, 4};
----------------
I like a separate test like this one, but using an `input_iterator`. It seems that code path is completely untested.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119589/new/

https://reviews.llvm.org/D119589