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

Arthur O'Dwyer via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Fri May 28 14:39:47 PDT 2021

Quuxplusone added inline comments.

Comment at: libcxx/test/support/test_iterators.h:636-637
 {   return !a.operator==(b); }
-// 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?

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. 

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list