[libcxx-commits] [PATCH] D150831: [libc++] Implement ranges::ends_with

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:62
+    auto __unwrapped2 = std::__unwrap_range(std::move(__first2), std::move(__last2));
+    return std::__equal_impl(
+        std::move(__unwrapped1.first),
----------------
ZijunZhao wrote:
> var-const wrote:
> > Question: why are we calling `__equal_impl` rather than `ranges::equal`?
> Because if we use `ranges::equal` , projections.pass.cpp and comparator.pass.cpp will fail for being copied.
Wrapping them in `std::ref` will probably solve the issue. `equal` has a bunch of optimizations based on the iterator type and it's likely we're skipping some logic by bypassing both `equal` and `__equal` and going straight to `__equal_impl`.


================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:81
+      _Range1&& __range1, _Range2&& __range2, _Pred __pred = {}, _Proj1 __proj1 = {}, _Proj2 __proj2 = {}) const {
+    auto __n1 = ranges::distance(__range1);
+    auto __n2 = ranges::distance(__range2);
----------------
var-const wrote:
> Can you just delegate to the non-range overload of `operator()`?
I would strongly advise to do so unless there's a compelling reason not to. It's better to avoid code duplication when possible in these cases.


================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:59
+      return false;
+    std::advance(__first1, (N1 - N2));
+    auto __unwrapped1 = std::__unwrap_range(std::move(__first1), std::move(__last1));
----------------
var-const wrote:
> philnik wrote:
> > philnik wrote:
> > > 
> > Potential optimization: If they are bidirectional and a common range we can step through back to front instead of iterating over the whole range.
> Looks like it's still `std::advance`.
Still seems not done?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:65
+  { // simple tests
+    int a[]          = {1, 2, 3, 4, 5, 6};
+    int p[]          = {1, 2, 3, 4, 5, 6};
----------------
I think these need to be reformatted (throughout the file).


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:154
+
+ { // range has zero length
+   int a[] = {};
----------------
Looks like this test is duplicated (see line 139).


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:169
+
+ { // subarray
+   int a[] = {0, 3, 5, 10, 3, 5, 8, 3, 5, 6};
----------------
How about:
```
The suffix occurs several time within the string, but not at the end.
```
?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:215
+
+  { // check that the predicate is used
+   int a[] = {3};
----------------
Duplicated.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:216
+  { // check that the predicate is used
+   int a[] = {3};
+   int p[] = {1};
----------------
How about:
```
int a[] = {5, 1, 3, 2, 7};
int p[] = {-2, -7};
auto pred = [](int l, int r) { return l * -1 == r; };
```
I'm sorry for partially pushing my personal preference, but what I like about this is that a) it compares more than 1 element and b) the comparison is more specific (only one value would compare equal, whereas an infinite number of values would compare greater than). I also find that using the negative complement of a number is an easy transformation to read (no need to do any mental math).


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:76
+    int p[]                               = {5, 6};
+    auto whole                            = std::ranges::subrange(Iter1(a), Sent1(Iter1(a + 6)));
+    auto suffix                           = std::ranges::subrange(Iter2(p), Sent2(Iter2(p + 2)));
----------------
var-const wrote:
> Consider using `std::end` to avoid dealing with indexes.
Seems not done?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:155
+
+ { // range has zero length
+  {
----------------
var-const wrote:
> Can we also check with 1-element range and (separately) 1-element suffix?
Seems not done?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:218
+constexpr bool test() {
+  types::for_each(types::forward_iterator_list<int*>{}, []<class I2>() {
+    types::for_each(types::forward_iterator_list<int*>{}, []<class I1>() {
----------------
ZijunZhao wrote:
> var-const wrote:
> > Shouldn't we also test with `input_iterator`s?
> In line 252, `forward_iterator_list` is tested and include all `input_iterators`  features. So `input_iterators` are tested?
It's kind of the opposite -- for an algorithm that supports input iterators, we need to make sure we don't accidentally end up relying on some features that are only available on forward iterators (or only on bidirectional iterators, etc.). In other words, we're checking that the algorithm only uses the very minimalistic interface provided by an `input_iterator` and nothing else. If we only test with `forward_iterator`s, for example, we wouldn't be able to catch if the algorithm presumes that calling `i++` on an iterator `i` returns a value (an input iterator can return anything, including `void`). 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150831/new/

https://reviews.llvm.org/D150831