[libcxx-commits] [PATCH] D123681: [libc++] Implement ranges::equal

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/ranges_equal.h:93
+    if constexpr (sized_range<_Range1> && sized_range<_Range2>) {
+      if (ranges::size(__range1) != ranges::size(__range2))
+        return false;
----------------
Mordante wrote:
> Interesting that the standard uses `distance` instead of `size`.
Same question -- what's the reason to use `size` as opposed to `distance` (which would be closer to the Standard)?


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.equal/ranges.equal.pass.cpp:75
+    {
+      int a[] = {1, 2, 3, 4};
+      int b[] = {1, 2, 3, 4};
----------------
Optional: a helper function would allow making the call sites quite a bit shorter:
```
assert(test({1, 2, 3, 4}, {1, 2, 3, 4});
assert(test({1, 2, 3, 4}, {2, 3, 4, 5}, [](int l, int r) { return l != r; });
...
// Comments can help distinguish parameters:
assert(test(/*a=*/ {1, 2, 3, 4}, /*b=*/ {2, 3, 4, 5}, /*pred=*/ [](int l, int r) { return l != r; });
```


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.equal/ranges.equal.pass.cpp:185
+  }
+}
+
----------------
Please also test when a) both ranges are empty and b) only one of the ranges is empty.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.equal/ranges.equal.pass.cpp:256
+      }
+    }
+    { // same size
----------------
Nit: please add a blank line after this line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123681