Details
- Reviewers
ldionne Mordante var-const jdoerfert - Group Reviewers
Restricted Project - Commits
- rGf8cbe3cdf024: [libc++] Implement ranges::remove{, _if}
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/__algorithm/ranges_remove_if.h | ||
---|---|---|
37 | By having an impl function, you can save 2 moves of pred and proj of the range overload, at the cost of having 2 extra moves of iterator/sentinel for iterator overload. is it worth it? sometimes moving iterators can be expensive, especially for iterators in the range adaptor (e.g. zip iterator) |
libcxx/include/CMakeLists.txt | ||
---|---|---|
98 | Please update the status page, the module map, and the private headers test file. | |
libcxx/include/__algorithm/ranges_remove.h | ||
38 | Nit: please move the iterators. | |
libcxx/include/__algorithm/ranges_remove_if.h | ||
38 | I found the name __end a little confusing -- it's not obvious how it is different from __last. How about new_end or similar? | |
libcxx/include/algorithm | ||
1290 | Please add synopsis. | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp | ||
17 | Nit: wrong synopsis. | |
48 | remove_if. | |
54 | Please also check permutable. | |
66 | Please add brief comments to explain what is being tested. | |
70 | Optional: consider using some intermediate variables to make this expression a little easier to parse. | |
92 | Can you please add a more complicated test case that contains the following?
Also might be worthwhile to check both the case where the last element in the range matches and the case when it doesn't match. | |
93 | It's not obvious from the name val that the predicate is < val -- perhaps name it max, cutoff{,_point} or similar? | |
102 | Nit: s/matche/match/. | |
102 | How about also No elements match? (for a non-trivial case with several elements) | |
132 | Nit: weird formatting. |
- Address comments
libcxx/include/__algorithm/ranges_remove_if.h | ||
---|---|---|
37 | I'd keep it for now as-is, but I think we want to review the stance on moving the iterators. I don't think anybody had zip_view and friends in mind when we decided to take the iterators by value. | |
38 | I think __new_end could also be a bit confusing, but definitely better than __end. | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp | ||
54 | That's done in lines 50 and 51, or am I missing something? | |
66 | What exactly are you asking for? Something like check that the expected outcome is the actual outcome isn't exactly useful, but I don't think there is anything more specific I could say. |
libcxx/include/module.modulemap.in | ||
---|---|---|
345 | Do you also need to add remove_if? | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp | ||
66 | Basically I meant something like // Iterator overload, // Range overload. | |
144 | Nit: can you add a little bit more context in addition to the link? |
Please update the status page, the module map, and the private headers test file.