[libcxx-commits] [PATCH] D117449: [libc++] Fix common_iterator for output_iterators

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

I continue to think all of these directions are better than the status quo. Consider me still "accept"ing.



================
Comment at: libcxx/include/__iterator/common_iterator.h:34-36
+  requires { typename iter_value_t<_Iter>; } &&
+  constructible_from<iter_value_t<_Iter>, iter_reference_t<_Iter>> &&
+  move_constructible<iter_value_t<_Iter>>;
----------------
Nit: Did you add line 34 to avoid prematurely instantiating `iter_reference_t`? Is it possible to regression-test for that? Should we? If removing line 34 would be conforming, I'd just do that.


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/plus_plus.pass.cpp:87-88
     commonIter1++;
     // Note: postfix operator++ returns void.
     // assert(*(commonIter1++) == 1);
     assert(*commonIter1 == 2);
----------------



================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/plus_plus.pass.cpp:154-155
+
+  // Increment a common_iterator<output_iterator>: iter_value_t is not valid for output iterators,
+  // so this gets tricky when we define operator++(int).
+  {
----------------
IIUC this comment is misleading. `iter_value_t` for an ordinary C++11 output iterator is just `ThatType::value_type`. It's only if the iterator doesn't define a `value_type`, or defines it as a non-object type, that `iter_value_t` might be ill-formed.
https://godbolt.org/z/1Ybjv7o9G
Our test iterator's `output_iterator<int*>::value_type` is `void`, so its `iter_value_t` is indeed ill-formed, but that's a special property of //our// `output_iterator`, not all output iterators in general.

I'd like to see test coverage for "output iterator with valid value_type" added to `iterator_traits.compile.pass.cpp` above, even if this means defining a little helper type a la the `OutIt` in my godbolt.


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/plus_plus.pass.cpp:174
+    iter++;
+    assert(iter == sent);
+  }
----------------
Consider checking `iter != sent` on at least one of the previous lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117449