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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision.
var-const added a comment.
This revision is now accepted and ready to land.

LGTM. There's just one blocking comment that should be a very straightforward fix and one completely optional suggestion, so I don't think this needs another round of review (unless something unexpected happens, of course). Thanks!



================
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;
----------------
philnik wrote:
> var-const wrote:
> > 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)?
> `ranges::distance` just forwards to `ranges::size` if it's a `sized_range`, so I don't see any reason for the additional indirection. If you'd rather have it spelled `ranges::distance` I can change it though.
I'd prefer to use `ranges::distance` just to be a little more consistent with the standard. That way, somebody reading the code could trivially verify that this part is correct, without looking up or recalling the details of how `distance` and `size` are defined. I don't think the extra indirection would cause any noticeable issues.


================
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};
----------------
var-const wrote:
> 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; });
> ```
Note: it's totally fine to ignore an optional suggestion, but in that case, please write a short comment to make it clear that you deliberately decided to not implement or postpone it. It would just make it more obvious to me whether you decided to not go with the suggestion or simply didn't get to this comment yet. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123681