Details
- Reviewers
ldionne Mordante var-const - Group Reviewers
Restricted Project - Commits
- rG7af89a379cce: [libc++] Implement ranges::fill{, _n}
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/__algorithm/ranges_fill.h | ||
---|---|---|
31 | Nit: I think contents of a class definition should be indented. | |
libcxx/include/algorithm | ||
179 | Nit: can you move fill_n so that it goes after fill, making it the same order as in the Standard? | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp | ||
15 | Please add SFINAE checks for output_iterator and sentinel_for (here and in the other test file). | |
18 | This test doesn't seem to check the case where dangling is returned. | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp | ||
28 | Can you also try with a non-trivially-copyable type? (e.g. to make sure that the implementation doesn't just unconditionally use memset) | |
29 | The usual nit about decltype(auto). | |
50 | Hmm, maybe I misunderstand something, but it seems like if elements were copied more than once, this test wouldn't catch that. | |
60 | Question: why is initializing with true necessary? |
- Address comments
libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp | ||
---|---|---|
28 | 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? | |
50 | Yep. Forgot the assert(!copied). | |
60 | It isn't. I think that was a leftover from a previous iteration of this test. |
Almost LGTM (just one remaining comment). Thanks!
libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp | ||
---|---|---|
28 | I'd prefer to have these test cases separate. For the non-trivially-copyable test, I'd use something that contains a pointer to make sure pointers are deep-copied. |
libcxx/include/__algorithm/ranges_fill_n.h | ||
---|---|---|
29 | Is there any reason for not using std::__fill_n? |
Nit: I think contents of a class definition should be indented.