Page MenuHomePhabricator

[libc++] Implement ranges::fill{, _n}
ClosedPublic

Authored by philnik on Apr 10 2022, 2:27 AM.

Details

Reviewers
ldionne
Mordante
var-const
Group Reviewers
Restricted Project
Commits
rG7af89a379cce: [libc++] Implement ranges::fill{, _n}

Diff Detail

Event Timeline

philnik created this revision.Apr 10 2022, 2:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2022, 2:27 AM
Herald added a subscriber: mgorny. · View Herald Transcript
philnik requested review of this revision.Apr 10 2022, 2:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2022, 2:27 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 421777.Apr 10 2022, 2:30 AM
  • Add Synopsis and link
var-const requested changes to this revision.Apr 11 2022, 2:18 PM
var-const added inline comments.
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?

This revision now requires changes to proceed.Apr 11 2022, 2:18 PM
philnik updated this revision to Diff 422317.Apr 12 2022, 1:02 PM
philnik marked 7 inline comments as done.
  • 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.

var-const requested changes to this revision.Apr 27 2022, 7:08 PM

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.

This revision now requires changes to proceed.Apr 27 2022, 7:08 PM
philnik updated this revision to Diff 429540.May 15 2022, 8:52 AM
philnik marked 2 inline comments as done.
  • Rebased
  • Address comments
philnik updated this revision to Diff 431143.May 21 2022, 9:27 AM
  • Try to fix CI
var-const accepted this revision.May 24 2022, 7:03 PM
This revision is now accepted and ready to land.May 24 2022, 7:03 PM
This revision was automatically updated to reflect the committed changes.
ldionne added inline comments.May 25 2022, 6:34 AM
libcxx/include/__algorithm/ranges_fill_n.h
29

Is there any reason for not using std::__fill_n?