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

Nikolas Klauser via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Tue Apr 12 13:02:59 PDT 2022

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.

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list