[libcxx-commits] [PATCH] D100587: [libc++][ranges] iterator.concept.sizedsentinel: sized_sentinel_for and disable_sized_sentinel_for.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

Code looks good modulo naming; the test drags in a lot of extra dependencies it doesn't need.



================
Comment at: libcxx/include/iterator:2600-2607
+template<class _Sentinel, class _Iter>
+concept sized_sentinel_for =
+  sentinel_for<_Sentinel, _Iter> &&
+  !disable_sized_sentinel_for<remove_cv_t<_Sentinel>, remove_cv_t<_Iter>> &&
+  requires(const _Iter& __i, const _Sentinel& __s) {
+    { __s - __i } -> same_as<iter_difference_t<_Iter>>;
+    { __i - __s } -> same_as<iter_difference_t<_Iter>>;
----------------
I'd prefer to see `_Sp` and `_Ip` just for consistency with line 2591.



================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sized_sentinel_for.compile.pass.cpp:42-47
+  friend constexpr bool operator==(nth_element_sentinel,
+                                   std::input_or_output_iterator auto);
+  friend constexpr long operator-(nth_element_sentinel,
+                                  std::input_or_output_iterator auto);
+  friend constexpr long operator-(std::input_or_output_iterator auto,
+                                  nth_element_sentinel);
----------------
Instead of `std::input_or_output_iterator auto`, how about `int*`:
```
struct IntSentinel {
    friend bool operator==(IntSentinel, int*);
    friend int operator-(IntSentinel, int*);
    friend int operator-(int*, IntSentinel);
};
static_assert(std::sized_sentinel_for<IntSentinel, int*>);
static_assert(!std::sized_sentinel_for<int*, IntSentinel>);  // IntSentinel is not an iterator
```


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sized_sentinel_for.compile.pass.cpp:65-71
+namespace std {
+
+template <>
+inline constexpr bool
+    disable_sized_sentinel_for<nth_element_sentinel, double*> = true;
+
+}
----------------
Please avoid reopening namespace std. Just
```
struct DoubleSentinel {
    friend bool operator==(DoubleSentinel, double*);
    friend int operator-(DoubleSentinel, double*);
    friend int operator-(double*, DoubleSentinel);
};
template<>
inline constexpr bool std::disable_sized_sentinel_for<DoubleSentinel, double*> = true;
    // notice the un-necessity of `namespace std {` here

static_assert(!std::sized_sentinel_for<DoubleSentinel, double*>);
```


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sized_sentinel_for.compile.pass.cpp:155-162
+// <filesystem>
+#ifndef _LIBCPP_HAS_NO_FILESYSTEM_LIBRARY
+static_assert(!std::sized_sentinel_for<std::filesystem::directory_iterator,
+                                       std::filesystem::directory_iterator>);
+static_assert(
+    !std::sized_sentinel_for<std::filesystem::recursive_directory_iterator,
+                             std::filesystem::recursive_directory_iterator>);
----------------
I don't think this test should depend on <filesystem>. (As below, all you're testing here is that you can't subtract `directory_iterator`s, and we knew that already.)


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.sentinel/sized_sentinel_for.compile.pass.cpp:186-190
+static_assert(check_not_sized_sentinel_for<std::unordered_map<int, int> >());
+static_assert(
+    check_not_sized_sentinel_for<std::unordered_multimap<int, int> >());
+static_assert(check_not_sized_sentinel_for<std::unordered_set<int> >());
+static_assert(check_not_sized_sentinel_for<std::unordered_multiset<int> >());
----------------
These initially confused me, but then I got it: it's because these iterators are merely bidirectional, not random-access, so they don't support `operator-`, so they're not "sized" sentinels. I would prefer to eliminate the dependency on the associative containers and write simply e.g.
```
#include "test_iterators.h"
static_assert(std::sized_sentinel_for<random_access_iterator<int*>, random_access_iterator<int*>>);
static_assert(!std::sized_sentinel_for<bidirectional_iterator<int*>, bidirectional_iterator<int*>>);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100587