This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Adds a test for std::fill_n.
ClosedPublic

Authored by Mordante on Jan 15 2022, 4:54 AM.

Details

Reviewers
Quuxplusone
ldionne
Group Reviewers
Restricted Project
Commits
rG8f4a6187f2cb: [libc++] Adds a test for std::fill_n.
Summary

The function std::fill requires a ForwardIterator, but std::fill_n
only requires an OutputIterator. Adds a test to validate std::fill_n
works with an OutputIterator.

Noticed this while working on LWG3539
format_to must not copy models of output_iterator<const charT&>

Diff Detail

Event Timeline

Mordante requested review of this revision.Jan 15 2022, 4:54 AM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2022, 4:54 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added a subscriber: Quuxplusone.

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

libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp
42

assert(base(it) == ca + n);
and likewise below. All of our test iterators provide an ADL base, and we also provide one for pointers.

66–72

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. :))

ldionne accepted this revision.Jan 18 2022, 7:32 AM
This revision is now accepted and ready to land.Jan 18 2022, 7:32 AM

FWIW I agree with Arthur's comments (but he seems to always beat me to it). Thanks for fixing this.

Mordante marked 2 inline comments as done.Jan 18 2022, 10:24 AM
Mordante added inline comments.
libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp
42

Thanks for the info I didn't know that.

66–72

I've updated the style, but I keep the char(1). Not sure why this was done so I prefer to keep it that way.

Mordante marked 2 inline comments as done.Jan 18 2022, 10:26 AM
This revision was automatically updated to reflect the committed changes.