[libcxx-commits] [PATCH] D123462: [libc++] Implement ranges::fill{, _n}

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/ranges_fill.h:30
+struct __fn {
+template <class _Type, output_iterator<const _Type&> _Iter, sentinel_for<_Iter> _Sent>
+_LIBCPP_HIDE_FROM_ABI constexpr
----------------
Nit: I think contents of a class definition should be indented.


================
Comment at: libcxx/include/algorithm:170
+  template<class T, output_iterator<const T&> O>
+    constexpr O ranges::fill_n(O first, iter_difference_t<O> n, const T& value);    // since C++20
+
----------------
Nit: can you move `fill_n` so that it goes after `fill`, making it the same order as in the Standard?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp:14
+
+// template<class T, output_iterator<const T&> O, sentinel_for<O> S>
+//   constexpr O ranges::fill(O first, S last, const T& value);
----------------
Please add SFINAE checks for `output_iterator` and `sentinel_for` (here and in the other test file).


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp:17
+// template<class T, output_range<const T&> R>
+//   constexpr borrowed_iterator_t<R> ranges::fill(R&& r, const T& value);
+
----------------
This test doesn't seem to check the case where `dangling` is returned.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp:27
+  { // simple test
+    int a[3];
+    std::same_as<It> auto ret = std::ranges::fill_n(It(a), 3, 1);
----------------
Can you also try with a non-trivially-copyable type? (e.g. to make sure that the implementation doesn't just unconditionally use `memset`)


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp:28
+    int a[3];
+    std::same_as<It> auto ret = std::ranges::fill_n(It(a), 3, 1);
+    assert(std::all_of(a, a + 3, [](int i) { return i == 1; }));
----------------
The usual nit about `decltype(auto)`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp:49
+
+  { // check that every element is copied once
+    struct S {
----------------
Hmm, maybe I misunderstand something, but it seems like if elements were copied more than once, this test wouldn't catch that.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp:59
+    S a[5];
+    std::ranges::fill_n(a, 5, S {true});
+    assert(std::all_of(a, a + 5, [](S& s) { return s.copied; }));
----------------
Question: why is initializing with `true` necessary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123462