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

Thu Jan 27 07:56:27 PST 2022

ldionne added a comment.

The CI only failed on Windows due to missing space. I'm going to ship this now, and we can make adjustments post-commit if the discussion ends up bringing up additional issues.

================
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:
> ldionne wrote:
> > Quuxplusone wrote:
> > > ldionne wrote:
> > > > CaseyCarter wrote:
> > > > > 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.
> > > > Agreed, the comment was wrong, I'll fix it. However:
> > > > 
> > > > > 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.
> > > > 
> > > > The problem is that even though `iter_value_t<OutputIter>` *can* be non-`void`, `iter_value_t<common_iterator<OutputIter>>` will never be, because we are now properly constraining the `iterator_traits<common_iterator<...>>` specialization to only work on `input_iterator`s. So I can't add a test that checks that `iter_value_t<common_iterator<OutputIter>>` is going to be non-`void`.
> > > > 
> > > > Please LMK if you think I'm mistaken here.
> > > I haven't investigated so I assume you're not mistaken. Then it just sounds like my request shifts from test coverage for `common_iterator<OutputIterWithNonVoidValueType>::iterator_category`, to test coverage that `common_iterator<OutputIterWithNonVoidValueType>` is SFINAE-friendly ill-formed. I believe we have some prior art, shaped roughly like
> > > ```
> > > template<class T>
> > > concept HasCommonIterator = requires { typename std::ranges::common_iterator<T>; };
> > > static_assert(!HasCommonIterator<OutputIterWithNonVoidValueType>);
> > > ```
> > I'm not following. `common_iterator<OutputIterWithNonVoidValueType>` does work, it's only that you can't query its `iterator_traits`.
> > 
> > I added a test in `iterator_traits.compile.pass.cpp` which isn't the most useful (because it ends up basically testing the `iterator_traits` base template), but should clarify what I mean.
> > I'm not following. common_iterator<OutputIterWithNonVoidValueType> does work, it's only that you can't query its iterator_traits.
> 
> Well, now I'm confused. How can you have an iterator, that "works," but you can't query its iterator_traits? What does that even mean? I would think that by definition a "working iterator" must have iterator_traits that say it's an iterator. Otherwise it's not an iterator.
> 
> I pasted your NonVoidOutputIterator into a Godbolt; what happens after this patch? (Also I assume that spew on GCC indicates a libstdc++ bug @jwakely.) https://godbolt.org/z/KrxY6Wd97
> I'm surprised that `common_iterator<OI, ...>::value_type` is `void` instead of `OI::value_type`, but I think that's what the standard literally says, yeah.
> 
Sorry, I should have been precise. I don't mean that `iterator_traits<common_iterator<OutputIterWithNonVoidValueType>>` doesn't work. I mean that `iterator_traits<common_iterator<OutputIterWithNonVoidValueType>>::value_type` is `void`, despite `OutputIterWithNonVoidValueType::value_type` not being `void`. And yes, I think it's a bit silly, but it's definitely what the Standard says.

The reason is simply that `iterator_traits<common_iterator<It>>` requires `It` to be an `input_iterator`. If it is, then the `value_type` is properly forwarded through. If it isn't, then we end up falling back on the base template of `iterator_traits` (https://en.cppreference.com/w/cpp/iterator/iterator_traits):

> Otherwise, if `Iter` satisfies the exposition-only concept `__LegacyIterator`, the member types are declared as follows:
> ```
> Member type         Definition
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> difference_type     std::incrementable_traits<Iter>::difference_type if valid, otherwise void
> value_type          void
> pointer             void
> reference           void
> iterator_category   std::output_iterator_tag
> ```

That's my understanding anyways.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117449