[libcxx-commits] [PATCH] D103341: [libcxx][nfc] Cleanup cpp20_input_iterator.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/test/support/test_iterators.h:636-637
 {   return !a.operator==(b); }
 
-#ifdef TEST_SUPPORTS_RANGES
-
-// clang-format off
+#if TEST_STD_VER >= 14
 
----------------
cjdb wrote:
> Why? This is a move-only iterator, which only makes sense in ranges land.
It makes sense anywhere it compiles, which is to say >=14.


================
Comment at: libcxx/test/support/test_iterators.h:641-642
 struct cpp20_input_iterator {
-  using value_type = std::iter_value_t<I>;
-  using difference_type = std::iter_difference_t<I>;
+  using value_type = typename std::iterator_traits<I>::value_type;
+  using difference_type = typename std::iterator_traits<I>::difference_type;
   using iterator_concept = std::input_iterator_tag;
----------------
cjdb wrote:
> Why?
'14-compatibility.


================
Comment at: libcxx/test/support/test_iterators.h:663
+  constexpr I const& base() const& { return base_; }
+  constexpr I base() && { return std::move(base_); }
 
----------------
cjdb wrote:
> This overload needs to keep its `[[nodiscard]]`.
IMO, no, the [[nodiscard]] was just noise. We don't need to [[nodiscard]] everything in sight: we don't [[nodiscard]] `operator*`, for example, even though "obviously" any test code that actually discarded the result of `*it` would probably have a bug, just as much as any test code that actually discarded the result of `it.base()`.


================
Comment at: libcxx/test/support/test_iterators.h:665-676
+  struct sentinel {
+    friend cpp20_input_iterator;
+
+    sentinel() = default;
+    sentinel(I base) : base(base) {}
+
+  private:
----------------
cjdb wrote:
> Iterators don't have sentinel sub-types. We have a `sentinel_wrapper` for that.
Hm. I don't //particularly// dislike `sentinel_wrapper`. At worst it's a tiny bit more verbose: compare the angle-bracketiness of alternatives
```
some_function<cpp20_input_iterator<int*>, sentinel_wrapper<cpp20_input_iterator<int*>>>();
some_function<cpp20_input_iterator<int*>, cpp20_input_iterator<int*>::sentinel>();
```
(Right now `sentinel_wrapper` is used only in "range.iter.ops.advance/advance.pass.cpp".)

I'm okay eliminating this diff for now, pending some examples of how `sentinel{,_wrapper}` would be used in real test cases. Certainly if we're adding `sentinel` we should also be refactoring some existing tests to show how it's an improvement — if it's not an improvement then we shouldn't do it.


================
Comment at: libcxx/test/support/test_iterators.h:872-873
 
+  template <class T>
+  void operator,(T const &) DELETE_FUNCTION;
+
----------------
Please `void operator&() const DELETE_FUNCTION;` while you're at it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103341