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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

Thanks for working on such an ugly type. In addition to my inline comments, please add

- a module map entry
- iterator conformance tests



================
Comment at: libcxx/include/__iterator/common_iterator.h:107
+  {
+    _LIBCPP_ASSERT(holds_alternative<_Iter>(__hold_), "Preconditions violated.");
+    return *__unchecked_get<_Iter>(__hold_);
----------------
ldionne wrote:
> zoecarver wrote:
> > This should say something like "common_iterator not holding an iterator (holding a sentinel) must hold iterator."
> > 
> > 
> As discussed live: we can use a better message here.
How about `"Dereferencing a common_iterator is only possible if it is holding an iterator, but this one is holding a sentinel."`?


================
Comment at: libcxx/include/__iterator/common_iterator.h:272-274
+  using pointer = conditional_t<__common_iter_has_ptr_op<_Iter, _Sent>,
+                                typename __arrow_type_or_void<_Iter, _Sent>::type,
+                                void>;
----------------
tcanens wrote:
> This `conditional_t` seems unnecessary?
Seconded. Please use `_If`, if you can.


================
Comment at: libcxx/include/__iterator/common_iterator.h:35
+class common_iterator {
+  class __proxy {
+    friend common_iterator;
----------------
Can we define `__proxy` and `__postfix_proxy` out-of-line please?


================
Comment at: libcxx/include/__iterator/common_iterator.h:63
+public:
+  variant<_Iter, _Sent> __hold_;
+
----------------
This should be private. Also, `hold` is not a particularly descriptive name to me.


================
Comment at: libcxx/include/__iterator/common_iterator.h:122-124
+    (requires(const _I2& __i) { __i.operator->(); } ||
+     is_reference_v<iter_reference_t<_I2>> ||
+     constructible_from<iter_value_t<_I2>, iter_reference_t<_I2>>)
----------------
Can we get this into a named object please? Perhaps `__common_iterator_has_arrow`?


================
Comment at: libcxx/include/__iterator/common_iterator.h:153-155
+    } else if constexpr (requires (_Iter& __i) { { *__i++ } -> __referenceable; } ||
+                         !(constructible_from<iter_value_t<_Iter>, iter_reference_t<_Iter>> &&
+                           move_constructible<iter_value_t<_Iter>>)) {
----------------
Please name this.


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/plus_plus.pass.cpp:86
+    commonIter1++;
+    // Note: operator++ returns void.
+    // assert(*(commonIter1++) == 1);
----------------
Please prefix "postfix" to this sentence.


================
Comment at: libcxx/test/support/test_iterators.h:164
+template <class It>
+class no_default_constructor
+{
----------------
ldionne wrote:
> 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.
I think this functionality should instead be reflected in `cpp20_input_iterator` (and in a separate patch, if it breaks any code).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103335