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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_ends_with.h:55-56
+      _Proj2 __proj2 = {}) const {
+    auto N1 = ranges::distance(__first1, __last1);
+    auto N2 = ranges::distance(__first2, __last2);
+    if (N2 > N1)
----------------
The names have to be uglified.


================
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));
----------------



================
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));
----------------
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.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:45
+static_assert(!HasEndsWithIt<int*, int*, int*, SentinelForNotSemiregular>);
+static_assert(!HasEndsWithIt<int*, int*, int*, SentinelForNotWeaklyEqualityComparableWith>);
+
----------------
Let's also add tests for AlmostInputRanges which a valid sized sentinel.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:66
+constexpr void test_iterators() {
+  {// simply tests
+   {int a[] = {1, 2, 3, 4, 5, 6};
----------------
and two spaces/indentation


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:67
+  {// simply tests
+   {int a[] = {1, 2, 3, 4, 5, 6};
+    int p[] = {5, 6};
----------------



================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:83
+
+  {// prefix doesn't match
+   {
----------------
throughout


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.ends_with/ranges.ends_with.pass.cpp:227-247
+  { // check that std::invoke is used
+    struct S {
+      int i;
+
+      constexpr S identity() { return *this; }
+
+      constexpr bool compare(const S& s) { return i == s.i; }
----------------
Please use the `libcxx/test/std/algorithms/ranges_robust_against_omitting_invoke.pass.cpp` test instead.


================
Comment at: libcxx/test/std/algorithms/ranges_robust_against_differing_projections.pass.cpp:80
   test(std::ranges::starts_with, in, in2, eq, proj1, proj2);
-  // test(std::ranges::ends_with, in, in2, eq, proj1, proj2);
 #endif
----------------
Please update all the `robust_against_*` tests!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150831