[libcxx-commits] [PATCH] D106205: [libcxx][ranges] Add `counted_iterator`.

Thu Jul 22 13:06:56 PDT 2021

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.

================
Comment at: libcxx/include/__iterator/counted_iterator.h:42-45
+template<__concept_has_iterator_concept _Iter>
+struct __counted_iterator_concept<_Iter> {
+  using iterator_concept = typename _Iter::iterator_concept;
+};
----------------
And then you can remove the named concept above.

================
Comment at: libcxx/include/__iterator/counted_iterator.h:53
+
+template<__concept_has_iterator_category _Iter>
+struct __counted_iterator_category<_Iter> {
----------------
Ditto.

================
Comment at: libcxx/include/__iterator/counted_iterator.h:61
+
+template<input_iterator _Iter>
+struct __counted_iterator_value_type<_Iter> {
----------------
This should be constrained on `indirectly_readable`?

================
Comment at: libcxx/include/__iterator/counted_iterator.h:73
+public:
+  _Iter __current_ = _Iter();
+  iter_difference_t<_Iter> __count_ = 0;
----------------
`[[no_unique_address]]`?

================
Comment at: libcxx/include/__iterator/counted_iterator.h:79
+
+  constexpr counted_iterator() requires default_initializable<_Iter> = default;
+
----------------
This one should be annotated too.

================
Comment at: libcxx/include/__iterator/counted_iterator.h:82
+  _LIBCPP_HIDE_FROM_ABI
+  constexpr counted_iterator(_Iter __iter, iter_difference_t<_Iter> __n)
+   : __current_(_VSTD::move(__iter)), __count_(__n) {}
----------------
`_LIBCPP_ASSERT(__n >= 0, "message");`? Might apply elsewhere we have `Precondtions:` in the spec.

================
Comment at: libcxx/include/__iterator/counted_iterator.h:107
+  _LIBCPP_HIDE_FROM_ABI
+  constexpr iter_difference_t<_Iter> count() const noexcept { return __count_; }
+
----------------
Are you testing the `noexcept`-ness of this method in your tests? If not, you should, especially since it's in the spec.

================
Comment at: libcxx/include/__iterator/counted_iterator.h:117
+  _LIBCPP_HIDE_FROM_ABI
+  constexpr auto operator->() const noexcept
+    requires contiguous_iterator<_Iter>
----------------
Ditto for `noexcept` testing.

================
Comment at: libcxx/include/__iterator/counted_iterator.h:125
+  constexpr counted_iterator& operator++() {
+    ++__current_;
+    --__count_;
----------------
Precondition!

================
Comment at: libcxx/include/__iterator/counted_iterator.h:131-134
+  decltype(auto) operator++(int) {
+    --__count_;
+    try { return __current_++; }
+    catch(...) { ++__count_; throw; }
----------------
This needs to be conditional on `#if _LIBCPP_NO_EXCEPTIONS`.

================
Comment at: libcxx/include/__iterator/counted_iterator.h:200
+  {
+    return __lhs.__count_ - __rhs.__count_;
+  }
----------------
This should be `__rhs.__count_ - __lhs.__count_`. Also, if there's no test failing right now, we should add one that fails with this incorrect implementation.

================
Comment at: libcxx/include/__iterator/counted_iterator.h:222
+    __current_ -= __n;
+    __count_ += __n;
+    return *this;
----------------
Missing precondition check.

================
Comment at: libcxx/include/__iterator/counted_iterator.h:227
+  _LIBCPP_HIDE_FROM_ABI
+  constexpr decltype(auto) operator[](iter_difference_t<_Iter> __n) const
+    requires random_access_iterator<_Iter>
----------------
Suggestion: Move this right after `operator->` to match the order it's in the spec.

================
Comment at: libcxx/include/__iterator/counted_iterator.h:259
+  {
+    return ranges::iter_move(__i.__current_);
+  }
----------------
Precondition.

================
Comment at: libcxx/include/__iterator/counted_iterator.h:267
+  {
+    return ranges::iter_swap(__x.__current_, __y.__current_);
+  }
----------------
Precondition.

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/assign.pass.cpp:16
+//     constexpr counted_iterator& operator=(const counted_iterator<I2>& x);
+
+#include <iterator>
----------------
Here and in all the other tests where the tested function doesn't require more than `input_or_output_iterator`, I'd like us to pass the `input_or_output_iterator` archetype as well. That will ensure we don't use capabilities we're not allowed to in the implementation.

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/assign.pass.cpp:59
+    assert(iter1.count() == 8);
+    iter1 = iter2;
+    assert(iter1.base().base() == buffer + 2);
----------------
Can you do:

```
std::counted_iterator<...>& result = (iter1 = iter2);
```

and also `assert(&result == &iter1)`? That makes sure we return the correct object.

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/assign.pass.cpp:71
+    assert(iter1.count() == 6);
+  }
+
----------------
Can we also add a test for when `I == I2`, i.e. assigning two iterators of the same type?

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/assign.pass.cpp:79
+  static_assert(test());
+
+  return 0;
----------------
Can you add a test checking that `!std::is_assignable<counted-iterator1, counted-iterator2>` when the underlying iterators are not assignable? To ensure we properly constrain the template.

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/base.pass.cpp:25
+  {
+    std::counted_iterator iter(cpp20_input_iterator<int*>{buffer}, 8);
+    assert(iter.base().base() == buffer);
----------------
Suggestion: templatize that test and call it with the various iterator archetypes to remove a bit of duplication.

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/compare.pass.cpp:58
+
+      assert(iter1 == iter2);
+      ++iter1;
----------------
Suggestion: add `assert(iter2 == iter1)`

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/ctor.pass.cpp:13
+
+// constexpr counted_iterator() requires default_initializable<I> = default;
+// constexpr counted_iterator(I x, iter_difference_t<I> n);
----------------
Can you have three files here? `ctor.default.pass.cpp`, etc.

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/ctor.pass.cpp:107
+      assert(iter2.count() == 8);
+    }
+  }
----------------
For the converting constructor, let's add a `static_assert(!std::is_constructible<counted_iterator, something-it-should-not-be-constructible-from>)` to make sure we constrain the constructor properly.

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/deref.pass.cpp:57
+  }
+
+  return true;
----------------
Can we add a test that `*it` is SFINAE-d away when the underlying iterator isn't const-dereferenceable? To make sure we have the `requires dereferenceable<const I>` constraint.

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/iter_move.pass.cpp:45
+
+  return true;
+}
----------------
We should check the noexcept-ness too.

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/member_types.compile.pass.cpp:21
+// No value_type.
+struct OutputIterator {
+  using difference_type = int;
----------------
What you just told me: This isn't an actual `output_iterator` since it's not `indirectly_writable`. So we should rename it. Maybe `InputOrOutputArchetype` (with a comment like `// archetype for input_or_output_iterator`)?

Also, this makes me think we should probably move that type in `test_iterators.h` (even though it's not *technically* an iterator).

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/minus.pass.cpp:13-23
+// constexpr counted_iterator operator-(iter_difference_t<I> n) const
+//   requires random_access_iterator<I>;
+// template<common_with<I> I2>
+//   friend constexpr iter_difference_t<I2> operator-(
+//     const counted_iterator& x, const counted_iterator<I2>& y);
+// friend constexpr iter_difference_t<I> operator-(
+//   const counted_iterator& x, default_sentinel_t);
----------------
Please split (you can leave the default_sentinel overloads in the same file).

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/minus.pass.cpp:71
+    {
+      using counted_t = std::counted_iterator<random_access_iterator<int*>>;
+      std::counted_iterator iter(random_access_iterator<int*>{buffer + 2}, 6);
----------------
`using Counted`?

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/minus.pass.cpp:72
+      using counted_t = std::counted_iterator<random_access_iterator<int*>>;
+      std::counted_iterator iter(random_access_iterator<int*>{buffer + 2}, 6);
+      assert(iter - 2 == counted_t(random_access_iterator<int*>{buffer}, 8));
----------------
And here you can declare it as `Counted iter` instead of using CTAD.

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/minus.pass.cpp:86
+
+      ASSERT_SAME_TYPE(decltype(iter - 2), std::remove_const_t<counted_t>);
+    }
----------------
`using Counted = std::counted_iterator<...>` (without the `const`).

Then you just declare `const Counted iter(...);`, and you can get rid of the `std::remove_const_t`. WDYT?

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/minus.pass.cpp:149
+    }
+    // Note: not using random_access_iterator here.
+    {
----------------
Can you add the reason why?

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/minus_minus.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
We typically call those `increment.pass.cpp` and `decrement.pass.cpp`.

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/plus_plus.pass.cpp:25
+template <class It>
+class ThrowsOnCopy
+{
----------------
Why don't you throw on `operator++` instead? It seems like the try-catch in `operator++(int)` is probably meant to catch that more than throwing on copy (even though both are equally important).

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/plus_plus.pass.cpp:113
+    try {
+      auto other = iter++;
+      (void)other;
----------------
Why not just `iter++`, without assigning the result?

================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/plus_plus.pass.cpp:120-129
+    try {
+      auto other = ++iter;
+      (void)other;
+      assert(false);
+    } catch (int x) {
+      assert(x == 42);
+      // TODO: LWG issue! This should be 8.
----------------
I don't think you're testing anything of `counted_iterator` here, so I think we should remove this test altogether.

================
Comment at: libcxx/test/support/test_iterators.h:701

-  cpp20_input_iterator() = default;
+  cpp20_input_iterator() = delete;

----------------
Nice catch!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106205