This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges] Implement `ranges::replace_copy{,_if}`.
ClosedPublic

Authored by var-const on Jul 14 2022, 3:04 PM.

Diff Detail

Event Timeline

philnik created this revision.Jul 14 2022, 3:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 3:04 PM
philnik requested review of this revision.Jul 14 2022, 3:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 3:04 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const retitled this revision from [libc++] Implement ranges::replace{, _if} to [libc++] Implement ranges::replace_copy{, _if}.Jul 14 2022, 3:15 PM

@philnik Please amend the description in git, should be replace_copy, not replace.

var-const added inline comments.Jul 14 2022, 3:19 PM
libcxx/include/algorithm
848

Please update the synopsis (you can copy the synopsis lines from the tests).

libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp
47 ↗(On Diff #444808)

This looks like an unintended change?

@philnik Please amend the description in git, should be replace_copy, not replace.

What do you mean exactly? The commit message gets updated automatically to the Differential title and description when landing if you mean that.

@philnik Please amend the description in git, should be replace_copy, not replace.

What do you mean exactly? The commit message gets updated automatically to the Differential title and description when landing if you mean that.

Yeah, basically I just meant to make sure the description of the merged patch is correct. If that's the case, never mind.

huixie90 accepted this revision.Jul 15 2022, 6:35 AM
huixie90 added inline comments.
libcxx/include/__algorithm/ranges_replace_copy.h
1

could you please update the status csv file?

40–54

puzzle: why does clang-format add two spaces here. I was told don't have spaces here

libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges_replace_copy.pass.cpp
68–69

since the signature of the algorithm have OldType and NewType, it would be good to have at least one test to test when the old_value and new_value have different types

I just looked at it mainly out of curiosity, but I didn't do a in-depth review. But some comments.

libcxx/.clang-format
71 ↗(On Diff #444808)

Please remove this, it should be in a separate patch. Which I did in D129441 on your request.

libcxx/include/__algorithm/ranges_replace_copy.h
40–54

This is the inner namespace indention clang-format setting. We're considering to remove that.

libcxx/test/libcxx/algorithms/ranges_robust_against_copying_projections.pass.cpp
217

It would be nice to commit the upper and lower bound and search separately.

philnik updated this revision to Diff 445334.Jul 17 2022, 11:04 AM
philnik marked 8 inline comments as done.
  • Address comments
libcxx/.clang-format
71 ↗(On Diff #444808)

Yes, sorry. I forgot to revert this change before committing.

philnik updated this revision to Diff 445454.Jul 18 2022, 4:29 AM
  • Try to fix CI
var-const requested changes to this revision.Jul 19 2022, 8:30 PM

All comments are pretty minor.

libcxx/include/__algorithm/ranges_replace_copy_if.h
44

Optional: consider a blank line above this line.

48

Nit: can you add a blank line above this line?

libcxx/include/algorithm
867

Please also add the definitions of the *_result types to the synopsis.

libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges_replace_copy.pass.cpp
41

s/RemoveIf/ReplaceCopy/.

82

Should this be replace_copy_result? Also, you might need to update ranges_result_alias_declarations.compile.pass.cpp.

203

Optional: it would be convenient to copy the exact requirement from the standard here.

libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges_replace_copy_if.pass.cpp
45

Same as the other file, the name needs to be updated.

58

s/ReplaceCopy/ReplaceCopyIf/.

89

Same nit re. return type.

219

I think you also need to check the number of predicate invocations. There's some prior art in test_support/counting_predicates.h.

This revision now requires changes to proceed.Jul 19 2022, 8:30 PM
var-const commandeered this revision.Aug 1 2022, 12:54 AM
var-const edited reviewers, added: philnik; removed: var-const.
var-const updated this revision to Diff 448940.Aug 1 2022, 1:38 AM
  • rebase on main;
  • add the new algorithms to all the robust test suites;
  • address existing feedback;
  • add more SFINAE tests;
  • test the number of predicate invocations in complexity tests.
var-const updated this revision to Diff 448961.Aug 1 2022, 2:11 AM

Fix forwarding in tests.

ldionne accepted this revision.Aug 2 2022, 2:15 PM
This revision is now accepted and ready to land.Aug 2 2022, 2:15 PM
var-const retitled this revision from [libc++] Implement ranges::replace_copy{, _if} to [libc++][ranges] Implement `ranges::replace_copy{, _if}`.Aug 2 2022, 10:27 PM
var-const retitled this revision from [libc++][ranges] Implement `ranges::replace_copy{, _if}` to [libc++][ranges] Implement `ranges::replace_copy{,_if}`.
var-const retitled this revision from [libc++][ranges] Implement `ranges::replace_copy{,_if}` to [libc++][ranges] Implement `ranges::replace_copy{,_if}`..
This revision was automatically updated to reflect the committed changes.