[libcxx-commits] [PATCH] D103335: [libcxx][ranges] Adds `common_iterator`.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a subscriber: mpark.
ldionne added a comment.
This revision now requires changes to proceed.

Comments per the over-the-shoulder review we just did.



================
Comment at: libcxx/include/__iterator/common_iterator.h:31-38
+// __unchecked_get is the same as std::get, except, if the wrong type is provided, it will
+// assert rather than throwing or returning nullptr. This makes it faster than std::get.
+template <size_t _Ip, class _Vp>
+inline _LIBCPP_INLINE_VISIBILITY
+constexpr auto&& __unchecked_get(_Vp&& __v) noexcept {
+  using __variant_detail::__access::__variant;
+  return __variant::__get_alt<_Ip>(_VSTD::forward<_Vp>(__v)).__value;
----------------
This should be defined in `<variant>`.

Also, I would word the comment differently (because assertions are disabled in most builds):

> __unchecked_get is the same as std::get, except, it is UB to use it with the wrong type [...]



================
Comment at: libcxx/include/__iterator/common_iterator.h:92
+  constexpr common_iterator(const common_iterator<_I2, _S2>& __other)
+    : __hold_(in_place_index<__other.__hold_.index()>, __other.__hold_) {}
+
----------------
I'm pretty sure this shouldn't even compile since we're passing the runtime `__other.__hold_.index()` as a non-type template argument, which expects a constant expression. This means that this isn't tested.

I can think of two ways of implementing this. First (pseudo-code):

```
__hold_([]() -> variant<_Iter, _Sent> {
  if (__other.__hold_.index() == 0) return {in_place_index<0>, __other.__hold_};
  else                                               return {in_place_index<1>, __other.__hold_};
  _LIBCPP_UNREACHABLE();
}())
```

The second way would be to use `variant<monostate, _Iter, _Sent>` and to "default-construct" the `monostate` when entering this constructor, and then do the `if-else` dance above in the constructor body. Otherwise, we'll have to default-construct one of the `variant` alternatives, which we don't want to do (BTW it would be awesome if you could add a test for that).

Pinging @mpark in case he has better ideas.


================
Comment at: libcxx/include/__iterator/common_iterator.h:97
+             assignable_from<const _I2, _Iter> && assignable_from<const _S2&, _Sent>
+  common_iterator& operator=(const common_iterator<_I2, _S2>& __other) {
+    if (__hold_.index() == __other.__hold_.index())
----------------
Could we add `_LIBCPP_ASSERT(!__other.__hold_­.valueless_­by_­exception())` here since it's a precondition in the spec? Also in the other places where it applies.


================
Comment at: libcxx/include/__iterator/common_iterator.h:99
+    if (__hold_.index() == __other.__hold_.index())
+      __hold_ = __other.__hold_;
+    else
----------------
This should be `std::get<index>(__hold_) = std::get<index>(__other.__hold_);`.


================
Comment at: libcxx/include/__iterator/common_iterator.h:101
+    else
+      __hold_.emplace<__hold_.index()>(__other.__hold_);
+    return *this;
----------------
This is going to have the same problem as above, it won't compile. That means there's also a hole in our test coverage.


================
Comment at: libcxx/include/__iterator/common_iterator.h:105
+
+  decltype(auto) operator*() { return *__unchecked_get<_Iter>(__hold_); }
+  decltype(auto) operator*() const
----------------
Maybe add `_LIBCPP_ASSERT(_VSTD::holds_­alternative<_Iter>(__hold_))`?


================
Comment at: libcxx/include/__iterator/common_iterator.h:114
+     is_reference_v<iter_reference_t<_Iter>> ||
+     constructible_from<iter_value_t<_Iter>, iter_reference_t<_Iter>>) {
+    if constexpr (is_pointer_v<_Iter> || requires(const _Iter& __i) { __i.operator->(); })    {
----------------
Could you break that `{` onto a different line? It's kind of hard to see where the `requires` end and the function body begins.


================
Comment at: libcxx/include/__iterator/common_iterator.h:145
+  friend bool operator==(const common_iterator& __x, const common_iterator<_I2, _S2>& __y) {
+    if (__x.__hold_.index() == __y.__hold_.index())
+      return true;
----------------
Can we hoist the value of `xxxx.index()` into local variables? (here and below)


================
Comment at: libcxx/include/__iterator/common_iterator.h:156
+    requires sentinel_for<_Sent, _I2> && equality_comparable_with<_Iter, _I2>
+  friend bool operator==(const common_iterator& __x, const common_iterator<_I2, _S2>& __y) {
+    if (__x.__hold_.index() == 1 && __y.__hold_.index() == 1)
----------------
Please make sure you test all the branches below (here and in the other functions below too).


================
Comment at: libcxx/include/__iterator/common_iterator.h:202-204
+  requires {
+    typename iterator_traits<_Iter>::iterator_category;
+  } &&
----------------
This would fit nicely on a single line IMO.


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/iterator_traits.compile.pass.cpp:56
+    static_assert(std::same_as<IterTraits::difference_type, std::ptrdiff_t>);
+    static_assert(std::same_as<IterTraits::pointer, const Iter&>); // TODO: is this right?
+    static_assert(std::same_as<IterTraits::reference, int&>);
----------------
This doesn't look right. According to the spec (http://eel.is/c++draft/iterators.common#common.iter.types-1.3), this should either be the type of `operator->()` or `void`, but never `Iter const&`. This means you most likely have a bug in the implementation.

Also, please add a test where `pointer` ends up being `void`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103335