Page MenuHomePhabricator

[libc++] Implement ranges::replace{, _if}
ClosedPublic

Authored by philnik on May 24 2022, 3:11 AM.

Details

Reviewers
ldionne
Mordante
var-const
Group Reviewers
Restricted Project
Commits
rGff6d5dee713c: [libc++] Implement ranges::replace{, _if}

Diff Detail

Event Timeline

philnik created this revision.May 24 2022, 3:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 3:11 AM
Herald added a subscriber: mgorny. · View Herald Transcript
philnik requested review of this revision.May 24 2022, 3:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 3:11 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const requested changes to this revision.May 24 2022, 12:45 PM

The implementation looks great. Just need to add a few test cases and update the synopsis. Thanks!

libcxx/docs/Status/RangesAlgorithms.csv
51–53

Please don't forget to update this with an actual link.

libcxx/include/algorithm
1173

You probably need to update the synopsis as well.

libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp
15

Can you also add the synopsis so that the algorithm signature is easy to look up when reading the test file?

21

Can you also test the requires clause and parameter constraints to check that SFINAE works correctly?

23

Nit: we normally use a trailing underscore for private variables. How about renaming it (maybe to something like original, original_input, etc.)?

23

In the signature, T1 and T2 can be different types, but the current tests only cover the case when they are the same type. Can you also test the conversion?

26

Nit: decltype(auto).

27

Nit: the formatting looks a little weird here, perhaps align oldval and newval with Iter on the line above?

44

Optional: consider adding comments to imitate named arguments, something like:

test<Iter, Sent, 4>(/*input=*/{1, 2, 3, 4}, /*old=*/2, /*new=*/23, /*expected=*/{1, 23, 3, 4});
46

Can this argument be deduced? (in all cases except for 0, I guess)

55

Can you also add a test for when the range overload returns dangling?

82

A couple more tests that I think should be added:

  1. In the standard, the operator== (and the predicate for the _if version) isn't required to return a bool, just something convertible to a boolean. Can you check those cases?
  2. There's a complexity requirement on the predicate and the projection. I know it's trivial, but perhaps test that as well to be exhaustive?
libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace_if.pass.cpp
23

I think all the same comments as in ranges.replace.pass.cpp apply here.

This revision now requires changes to proceed.May 24 2022, 12:45 PM

The implementation looks great. Just need to add a few test cases and update the synopsis. Thanks!

Also, can you check if you need to update niebloid.compile.pass.cpp?

philnik updated this revision to Diff 434710.Jun 7 2022, 12:38 AM
philnik marked 13 inline comments as done.
  • Rebased
  • Address comments
philnik updated this revision to Diff 434712.Jun 7 2022, 12:42 AM
  • Add version guards
philnik added inline comments.Jun 7 2022, 1:26 AM
libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp
44

I'm using a struct with designated initializers. WDYT?

46

I tried it and couldn't get it to work.

var-const accepted this revision.Thu, Jun 9, 7:59 PM
var-const added inline comments.
libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp
44

It looks great, definitely better than comments.

46

Right -- looks like you would need to pass std::array{1, 2, 3, 4}. Let's leave as is.

This revision is now accepted and ready to land.Thu, Jun 9, 7:59 PM
This revision was automatically updated to reflect the committed changes.