Page MenuHomePhabricator

[libcxx] patch for implementing ranges::find_last
Needs ReviewPublic

Authored by phyBrackets on Oct 21 2022, 9:18 PM.

Details

Reviewers
philnik
ldionne
huixie90
bollu
Group Reviewers
Restricted Project
Summary

Diff Detail

Event Timeline

phyBrackets created this revision.Oct 21 2022, 9:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2022, 9:18 PM
phyBrackets requested review of this revision.Oct 21 2022, 9:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2022, 9:18 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
huixie90 requested changes to this revision.Oct 22 2022, 12:08 PM
huixie90 added a subscriber: huixie90.
huixie90 added inline comments.
libcxx/include/__algorithm/ranges_find_last.h
2

Please add tests for all the algorithms' behaviour and constraints.
You can take a look at this to get an idea about how we test algorithms
https://reviews.llvm.org/D130404

30

is the paper C++23? If so, should it be

#if _LIBCPP_STD_VER >= 23

same applies to other files in the patch

40

question. can we use static operator() now?

libcxx/include/__algorithm/ranges_find_last_if.h
37

There are several issues.

  1. In general __last is a sentinel (as supposed to be an iterator). The only thing you can do about it is to compare with an iterator. so you cannot use operator-- for a sentinel.
  2. The algorithm's requirement is forward_iterator (instead of bidirectional_iterator). operator-- does not exist for forward_iteartor. You will probably need to dispatch to different implementations depending on whether it is common_range, bidirectional_range, sized_sentinel_for/sized_range
This revision now requires changes to proceed.Oct 22 2022, 12:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2023, 9:31 PM
fsb4000 added a subscriber: fsb4000.Mar 9 2023, 1:18 AM
fsb4000 added inline comments.
libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if_not.pass.cpp
217

Should be static_assert?

fsb4000 added inline comments.Mar 9 2023, 1:19 AM
libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last.pass.cpp
246

static_assert

libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp
231

static_assert

fsb4000 added inline comments.Mar 9 2023, 1:21 AM
libcxx/docs/Status/Cxx2bPapers.csv
58

instead of

"","|ranges|"

should be

"17.0","|ranges|"

phyBrackets marked 4 inline comments as done.

Thanks for your patch. I only want over it lightly. I will do an full review after the current open issues are addressed.

libcxx/include/__algorithm/ranges_find_last.h
30

@phyBrackets can you mark items as done when they are done? That makes reviewing these changes a lot easier.

40

Why would that not be possible, I've seen this used in other ranges patches too.

56

Please run clang-format on new files, I'm quite sure this indention is off.

libcxx/include/__algorithm/ranges_find_last_if.h
39

Please don't use auto everywhere, this just makes things harder to read.
Please make sure all names are __uglified, for example result too,

libcxx/include/__algorithm/ranges_find_last_if_not.h
43

naming is hard, but let's try to use something better than just a number.
How about __not_pred since this is basically the inverse of the predicate given.

Another option is just not giving this predicate a name and write it in the return statement on the next line.

libcxx/include/algorithm
102

This indention is off.

104

Please align all these since entries.

libcxx/include/module.modulemap.in
365

Please fix the alignment of the {.

libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last.pass.cpp
82

Here you should use std::same_as<return type> auto method. For example have a look at
libcxx/test/std/ranges/range.adaptors/range.common.view/begin.pass.cpp

libcxx/test/std/algorithms/alg.nonmodifying/alg.find/ranges.find_last_if.pass.cpp
106

Please use clang-format on these tests, this looks wrong.

libcxx/utils/data/ignore_format.txt
119

The ignore list is there since we don't want to reformat our entire code base and have merge conflicts for existing patches. New files should always be formatted.

llvm/utils/gn/secondary/libcxx/include/BUILD.gn
170 ↗(On Diff #506629)

Typically we don't update the bazel files, there is a post-commit bot that does that.

phyBrackets marked 2 inline comments as done.
phyBrackets marked 8 inline comments as done.
phyBrackets marked an inline comment as done.