Details
- Reviewers
huixie90 philnik ldionne - Group Reviewers
Restricted Project - Commits
- rG93172c1c2b10: [libc++][ranges] Implement `ranges::replace_copy{,_if}`.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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 | ||
66–67 | 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 | ||
222 | It would be nice to commit the upper and lower bound and search separately. |
- Address comments
libcxx/.clang-format | ||
---|---|---|
71 ↗ | (On Diff #444808) | Yes, sorry. I forgot to revert this change before committing. |
All comments are pretty minor.
libcxx/include/__algorithm/ranges_replace_copy_if.h | ||
---|---|---|
44 | Optional: consider a blank line above this line. | |
47 | Nit: can you add a blank line above this line? | |
libcxx/include/algorithm | ||
737 | 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 | ||
39 | s/RemoveIf/ReplaceCopy/. | |
80 | Should this be replace_copy_result? Also, you might need to update ranges_result_alias_declarations.compile.pass.cpp. | |
170 | 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 | ||
42 | Same as the other file, the name needs to be updated. | |
55 | s/ReplaceCopy/ReplaceCopyIf/. | |
86 | Same nit re. return type. | |
186 | I think you also need to check the number of predicate invocations. There's some prior art in test_support/counting_predicates.h. |
- 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.
could you please update the status csv file?