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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
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);
----------------
var-const wrote:
> Can you also try with a non-trivially-copyable type? (e.g. to make sure that the implementation doesn't just unconditionally use `memset`)
I think we have this discussion in another PR as well. Would it be OK for you if I just add that it's testing non-trivially-copyable things to the description of `// check that every element is copied once`?


================
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 {
----------------
var-const wrote:
> Hmm, maybe I misunderstand something, but it seems like if elements were copied more than once, this test wouldn't catch that.
Yep. Forgot the `assert(!copied)`.


================
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; }));
----------------
var-const wrote:
> Question: why is initializing with `true` necessary?
It isn't. I think that was a leftover from a previous iteration of this test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123462