Page MenuHomePhabricator

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

Authored by philnik on Jun 26 2022, 2:24 PM.

Details

Diff Detail

Event Timeline

philnik created this revision.Jun 26 2022, 2:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2022, 2:24 PM
Herald added a subscriber: mgorny. · View Herald Transcript
philnik requested review of this revision.Jun 26 2022, 2:24 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
huixie90 added inline comments.
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)

var-const requested changes to this revision.Jun 29 2022, 2:30 AM
var-const added inline comments.
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?

  • two or more matching elements in a row;
  • a matching element followed by a non-matching element;
  • a matching element followed by several non-matching elements?

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.

This revision now requires changes to proceed.Jun 29 2022, 2:30 AM
philnik updated this revision to Diff 442246.Jul 5 2022, 3:07 AM
philnik marked 14 inline comments as done.
  • 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.

var-const accepted this revision.Jul 5 2022, 4:19 PM
var-const added inline comments.
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?

This revision is now accepted and ready to land.Jul 5 2022, 4:19 PM
philnik updated this revision to Diff 442515.Jul 6 2022, 4:39 AM
philnik marked 4 inline comments as done.
  • Address comments
philnik updated this revision to Diff 442521.Jul 6 2022, 5:03 AM
  • Fix CI
This revision was automatically updated to reflect the committed changes.