[libcxx-commits] [PATCH] D117395: [libc++] Adds a test for std::fill_n.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM % comments — please definitely fix the `base(x)` thing, but if you consider the other refactoring too much scope creep, OK.



================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp:55
+    Iter it = std::fill_n(Iter(ca), UDI(n), char(1));
+    assert(equal(it, ca + n));
     assert(ca[0] == 1);
----------------
`assert(base(it) == ca + n);`
and likewise below. All of our test iterators provide an ADL `base`, and we also provide one for pointers.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp:79-85
     const unsigned n = 4;
     int ia[n] = {0};
-    assert(std::fill_n(ia, UDI(n), static_cast<char>(1)) == std::next(ia, n));
+    assert(std::fill_n(ia, UDI(n), static_cast<char>(1)) == ia + 4);
     assert(ia[0] == 1);
     assert(ia[1] == 1);
     assert(ia[2] == 1);
     assert(ia[3] == 1);
----------------
Here and in all cases below, it would be nice to make it just
```
    int a[4] = {};
    assert(std::fill_n(a, UDI(4), char(1)) == a + 4);
    assert(a[0] == 1); // etc 
```
i.e. there's no reason for all these arrays to have names different from `a`, or for variable `n` to exist at all. (There also doesn't seem to be a reason for us to be filling //this// int array with `char(1)` instead of just `1`, but I didn't look closely. :))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117395