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

[Casey Carter via Phabricator via libcxx-commits]

CaseyCarter added a comment.

LGTM, with Arthur's comments. (Although I'm fine with the extra typename requirement in `__can_use_postfix_proxy`.)



================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/iterator_traits.compile.pass.cpp:55
+    static_assert(!HasIteratorConcept<IterTraits>);
+    static_assert(std::same_as<IterTraits::iterator_category, std::output_iterator_tag>);
+    static_assert(std::same_as<IterTraits::value_type, void>);
----------------
ldionne wrote:
> Quuxplusone wrote:
> > CaseyCarter wrote:
> > > ldionne wrote:
> > > > Quuxplusone wrote:
> > > > > Well this is weird. `non_const_deref_iterator<int*>::iterator_category` appears to be `input_iterator_tag`, but `std::common_iterator<Iter, sentinel_type<int*>>` is an output iterator instead?
> > > > > 
> > > > > Also, I'm seeing now that `non_const_deref_iterator` is a template, but it's used in only one place (with `int*`), so it can be massively simplified. Would you like me to just take this over and merge it into my existing D117400?
> > > > Yeah, I'd like to understand this part as well.
> > > `input_iterator` requires that `const` iterators are dereferenceable. Hopefully it's obvious that is not the case for `non_const_deref_iterator`, so it does not trigger the `iterator_traits<common_iterator<I, S>>` partial specialization once that partial specialization is properly constrained to require `input_iterator`.
> > I get everything you said, but I still don't get why "`Iter` doesn't satisfy `input_iterator`" should lead us to "`iterator_traits<CommonIter>::iterator_category` is defined and is equal to `output_iterator_tag`." Like, `Iter` isn't an output iterator //either//... is it? So the most logical outcomes (in increasing order of wild-west-ness) would have been for `CommonIter` to be ill-formed, or for `CommonIter` to be well-formed but not an iterator, or for `iterator_traits<CommonIter>::iterator_category` to be defined as equal to `iterator_traits<Iter>::iterator_category`. Having it just randomly equal to `output_iterator_tag` seems bizarre!
> > 
> > (I'm not disputing that it's probably //correct// according to C++20; I'm just confused about both the mental model and, literally, I don't see by what codepath we end up with `output_iterator_tag`. Not that I've looked hard at all.)
> Thanks for explaining @CaseyCarter , but in that case I fail to see the point of this test at all. Instead, why don't we use `common_iterator` with an `output_iterator`, and then simply check that `IterTraits::iterator_concept` isn't defined (which would effectively be a regression test for your change). In other words:
> 
> ```
> using Iter = output_iterator<int*>;
> using CommonIter = std::common_iterator<Iter, sentinel_type<int*>>;
> using IterTraits = std::iterator_traits<CommonIter>;
> 
> static_assert(!HasIteratorConcept<IterTraits>);
> static_assert(std::same_as<IterTraits::iterator_category, std::output_iterator_tag>);
> // etc...
> ```
> 
> Do you agree that is what we want? Note that this won't work today because it appears that `common_iterator<output_iterator<int*>>` doesn't work at all today, but we can and should fix that separately (I have a WIP patch locally).
> 
`common_iterator` assumes that non-`input_iterator`s must be `output_iterator`s. Testing with `non_const_deref_iterator` is equivalent to testing with `output_iterator` if we never dereference an iterator. `common_iterator` doesn't intend to be as general as possible: it exists only to adapt "new" iterator+sentinels into "old" iterator pairs. Consequently it has little patience with types that don't satisfy the concepts.


================
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).
+  {
----------------
Quuxplusone wrote:
> 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.
"need not be valid for output iterators" would be fine here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117449