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

[Louis Dionne via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/include/__iterator/common_iterator.h:33-41
+  union _Hold {
+    _Iter __iter; // __index == 0;
+    _Sent __sent; // __index == 1;
+
+    _Hold() = default;
+    constexpr _Hold(_Iter __iter) : __iter(_VSTD::move(__iter)) {}
+    constexpr _Hold(_Sent __sent) : __sent(_VSTD::move(__sent)) {}
----------------
Reminder that this should be using `std::variant`.


================
Comment at: libcxx/include/__iterator/common_iterator.h:115
+    } else if constexpr (requires (_Iter& __i) { { *__i++ } -> __referenceable; } ||
+                         !constructible_from<iter_value_t<_Iter>, iter_reference_t<_Iter>>) {
+      return __u.__iter++;
----------------
http://eel.is/c++draft/iterators.common#common.iter.nav-5 says:

> Otherwise, if `requires (I& i) { { *i++ } -> can-reference; }` is `true` or `constructible_­from<iter_­value_­t<I>, iter_­reference_­t<I>> && move_­constructible<iter_­value_­t<I>>` is `false`, equivalent to: `return get<I>(v_)++;`

We're missing `&& move_constructible<...>` here?


================
Comment at: libcxx/include/__iterator/common_iterator.h:118
+    } else {
+      __postfix_proxy __p(*this);
+      ++*this;
----------------
This should be `__postfix_proxy __p(**this);` (double-dereference to get the value pointed-to by the iterator).

It also means you have a hole in your coverage since your tests were passing, i.e. this branch of the `if constexpr` was never exercised. Please make sure all branches of the `if constexpr` are exercised (here and in `operator->` above too).


================
Comment at: libcxx/include/__iterator/common_iterator.h:99
+             assignable_from<const _I2, _Iter> && assignable_from<const _S2&, _Sent>
+  constexpr common_iterator& operator=(const common_iterator<_I2, _S2>& __other) {
+    __u = __other.__u;
----------------
cjdb wrote:
> zoecarver wrote:
> > Note: all of these constexprs aren't required by the standard, but I'm filing an lwg issue for it. 
> Please link the issue in this PR before submitting (it doesn't need to be in the patch itself).
I think we should not make them `constexpr` until the LWG issue has been accepted and resolved. I strongly suspect the reason why they are not `constexpr` is that the type is expected to use `variant`, which isn't `constexpr` yet. You can structure the tests so that `constexpr` is easy to test when we add it, but please don't add it in the implementation yet.


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/arrow.pass.cpp:26
+  {
+    auto iter1 = simple_iterator<int*>(buffer);
+    auto commonIter1 = std::common_iterator<decltype(iter1), sentienl_type<int*>>(iter1);
----------------
Here and for `operator++(int)`, I'd like you to explicitly call out which part is testing each sub-bullet in the spec (which maps to the `if constexpr` in the implementation).


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/assign.pass.cpp:30
+    auto iter1 = cpp17_input_iterator<int*>(buffer);
+    auto commonIter1 = std::common_iterator<decltype(iter1), sentienl_type<int*>>(iter1);
+    auto commonIter2 = std::common_iterator<decltype(iter1), sentienl_type<int*>>(cpp17_input_iterator<int*>(buffer + 1));
----------------
You have a typo in `sentienl_type`.


================
Comment at: libcxx/test/support/test_iterators.h:164
+template <class It>
+class no_default_constructor
+{
----------------
zoecarver wrote:
> I think this will be a very helpful type to have now that we need to test the default constructibility of all our views/iterators.
I agree, but I would instead call it something like `non_default_constructible_iterator`. Or whatever you prefer, but I think it's important for the name to reflect the fact that it's an iterator.


================
Comment at: libcxx/test/support/test_iterators.h:170
+public:
+    typedef          std::forward_iterator_tag                 iterator_category;
+    typedef typename std::iterator_traits<It>::value_type      value_type;
----------------
Shouldn't this be `std::iterator_traits<It>::iterator_category`? Actually, we could perhaps do this instead:

```
template<class It>
struct std::iterator_traits<no_default_constructor<It>> : std::iterator_traits<It> { };
```

Then, could we simply drop all the typedefs in `no_default_constructor`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103335