Re: [PATCH] libstdc++: Implement P2259R1 changes [PR95983]

[Patrick Palka <ppalka@redhat.com>]

On Tue, 20 Apr 2021, Jonathan Wakely wrote:

> On 19/04/21 22:25 -0400, Patrick Palka via Libstdc++ wrote:
> > This implements the wording changes of P2259R1 "Repairing input range
> > adaptors and counted_iterator", which resolves LWG 3283, 3289 and 3408.
> > 
> > The wording changes are relatively straightforward, but they require
> > some boilerplate to implement: the changes to make a type alias
> > "conditionally present" in some iterator class are realized by defining
> > a base class template and an appropriately constrained partial
> > specialization thereof that contains the type alias, and having the
> > iterator class derive from this base class.  Sometimes the relevant
> > condition depend on members from the iterator class, but because a
> > class is incomplete when instantiating its bases, the constraints on
> > the partial specialization can't use anything from the iterator class.
> > This patch works around this by hoisting these members out to the
> > enclosing scope (e.g. transform_view::_Iterator::_Base is hoisted out
> > to transform_view::_Base so that transform_view::__iter_cat can use it).
> > 
> > This patch also implements the proposed resolution of LWG 3291 to rename
> > iota_view::iterator_category to iota_view::iterator_concept, which was
> > previously problematic due to LWG 3408.
> > 
> > Tested on x86_64-pc-linux-gnu.
> > 
> > libstdc++-v3/ChangeLog:
> > 
> > 	PR libstdc++/95983
> > 	* include/bits/stl_iterator.h (__detail::__move_iter_cat):
> > 	Define.
> > 	(move_iterator): Derive from the above in C++20 in order to
> > 	conditionally define iterator_category as per P2259.
> > 	(move_iterator::__base_cat): No longer used, so remove.
> > 	(move_iterator::iterator_category): Remove in C++20.
> > 	(__detail::__common_iter_use_postfix_proxy): Define.
> > 	(common_iterator::_Proxy): Rename to ...
> > 	(common_iterator:__arrow_proxy): ... this.
> > 	(common_iterator::__postfix_proxy): Define as per P2259.
> > 	(common_iterator::operator->): Adjust.
> > 	(common_iterator::operator++): Adjust as per P2259.
> > 	(iterator_traits<common_iterator>::_S_iter_cat): Define.
> > 	(iterator_traits<common_iterator>::iterator_category): Change as
> > 	per P2259.
> > 	(__detail::__counted_iter_value_type): Define.
> > 	(__detail::__counted_iter_concept): Define.
> > 	(__detail::__counted_iter_cat): Define.
> > 	(counted_iterator): Derive from the above three classes in order
> > 	to conditionally define value_type, iterator_concept and
> > 	iterator category respectively as per P2259.
> > 	(counted_iterator::operator->): Define as per P2259.
> > 	(incrementable_traits<counted_iterator>): Remove as per P2259.
> > 	(iterator_traits<counted_iterator>): Adjust as per P2259.
> > 	* include/std/ranges (__detail::__iota_view_iter_cat): Define.
> > 	(iota_view::_Iterator): Derive from the above in order to
> > 	conditionally define iterator_category as per P2259.
> > 	(iota_view::_S_iter_cat): Rename to ...
> > 	(iota_view::_S_iter_concept): ... this.
> > 	(iota_view::iterator_concept): Use it to apply LWG 3291 changes.
> > 	(iota_view::iterator_category): Remove.
> > 	(__detail::__filter_view_iter_cat): Define.
> > 	(filter_view::_Iterator): Derive from the above in order to
> > 	conditionally define iterator_category as per P2259.
> > 	(filter_view::_Iterator): Move to struct __filter_view_iter_cat.
> > 	(filter_view::_Iterator::iterator_category): Remove.
> > 	(transform_view::_Base): Define.
> > 	(transform_view::__iter_cat): Define.
> > 	(transform_view::_Iterator): Derive from the above in order to
> > 	conditionally define iterator_category as per P2259.
> > 	(transform_view::_Iterator::_Base): Just alias
> > 	transform_view::_Base.
> > 	(transform_view::_Iterator::_S_iter_cat): Move to struct
> > 	transform_view::__iter_cat.
> > 	(transform_view::_Iterator::iterator_category): Remove.
> > 	(transform_view::_Sentinel::_Base): Just alias
> > 	transform_view::_Base.
> > 	(join_view::_Base): Define.
> > 	(join_view::_Outer_iter): Define.
> > 	(join_view::_Inner_iter): Define.
> > 	(join_view::_S_ref_is_glvalue): Define.
> > 	(join_view::__iter_cat): Define.
> > 	(join_view::_Iterator): Derive from it in order to conditionally
> > 	define iterator_category as per P2259.
> > 	(join_view::_Iterator::_Base): Just alias join_view::_Base.
> > 	(join_view::_Iterator::_S_ref_is_glvalue): Just alias
> > 	join_view::_S_ref_is_glvalue.
> > 	(join_view::_Iterator::_S_iter_cat): Move to struct
> > 	transform_view::__iter_cat.
> > 	(join_view::_Iterator::_Outer_iter): Just alias
> > 	join_view::_Outer_iter.
> > 	(join_view::_Iterator::_Inner_iter): Just alias
> > 	join_view::_Inner_iter.
> > 	(join_view::_Iterator::iterator_category): Remove.
> > 	(join_view::_Sentinel::_Base): Just alias join_view::_Base.
> > 	(__detail::__split_view_outer_iter_cat): Define.
> > 	(__detail::__split_view_inner_iter_cat): Define.
> > 	(split_view::_Base): Define.
> > 	(split_view::_Outer_iter): Derive from __split_view_outer_iter_cat
> > 	in order to conditionally define iterator_category as per P2259.
> > 	(split_view::_Outer_iter::iterator_category): Remove.
> > 	(split_view::_Inner_iter): Derive from __split_view_inner_iter_cat
> > 	in order to conditionally define iterator_category as per P2259.
> > 	(split_view::_Inner_iter::_S_iter_cat): Move to
> > 	__split_view_inner_iter_cat.
> > 	(split_view::_Inner_iter::iterator_category): Remove.
> > 	(elements_view::_Base): Define.
> > 	(elements_view::__iter_cat): Define.
> > 	(elements_view::_Iterator): Derive from the above in order to
> > 	conditionall define iterator_category as per P2259.
> > 	(elements_view::_Iterator::_Base): Just alias
> > 	elements_view::_Base.
> > 	(elements_view::_Iterator::_S_iter_concept)
> > 	(elements_view::_Iterator::iterator_concept): Define as per
> > 	P2259.
> > 	(elements_view::_Iterator::iterator_category): Remove.
> > 	(elements_view::_Sentinel::_Base): Just alias
> > 	elements_view::_Base.
> > 	* testsuite/24_iterators/headers/iterator/synopsis_c++20: Adjust
> > 	constraints on iterator_traits<counted_iterator>.
> > 	* testsuite/std/ranges/p2259.cc: New test.
> > ---
> > libstdc++-v3/include/bits/stl_iterator.h      | 128 ++++++--
> > libstdc++-v3/include/std/ranges               | 299 ++++++++++++------
> > .../headers/iterator/synopsis_c++20.cc        |   1 +
> > libstdc++-v3/testsuite/std/ranges/p2259.cc    |  91 ++++++
> > 4 files changed, 412 insertions(+), 107 deletions(-)
> > create mode 100644 libstdc++-v3/testsuite/std/ranges/p2259.cc
> > 
> > diff --git a/libstdc++-v3/include/bits/stl_iterator.h
> > b/libstdc++-v3/include/bits/stl_iterator.h
> > index dc8b101e8f8..049f83cff90 100644
> > --- a/libstdc++-v3/include/bits/stl_iterator.h
> > +++ b/libstdc++-v3/include/bits/stl_iterator.h
> > @@ -1302,6 +1302,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >     };
> > #endif // C++20
> > 
> > +  namespace __detail
> > +  {
> > +#if __cplusplus > 201703L && __cpp_lib_concepts
> > +    template<typename _Iterator>
> > +      struct __move_iter_cat
> > +      { };
> > +
> > +    template<typename _Iterator>
> > +      requires requires { typename
> > iterator_traits<_Iterator>::iterator_category; }
> > +      struct __move_iter_cat<_Iterator>
> > +      {
> > +	using iterator_category
> > +	  = __clamp_iter_cat<typename
> > iterator_traits<_Iterator>::iterator_category,
> > +			     random_access_iterator_tag>;
> > +      };
> > +#endif
> > +  }
> > +
> >   // 24.4.3  Move iterators
> >   /**
> >    *  Class template move_iterator is an iterator adapter with the same
> > @@ -1313,13 +1331,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >    */
> >   template<typename _Iterator>
> >     class move_iterator
> > +#if __cplusplus > 201703L && __cpp_lib_concepts
> > +      : public __detail::__move_iter_cat<_Iterator>
> > +#endif
> >     {
> >       _Iterator _M_current;
> > 
> >       using __traits_type = iterator_traits<_Iterator>;
> > -#if __cplusplus > 201703L && __cpp_lib_concepts
> > -      using __base_cat = typename __traits_type::iterator_category;
> > -#else
> > +#if ! (__cplusplus > 201703L && __cpp_lib_concepts)
> 
> These preprocessor conditionals would be simpler if we just used
> __cpp_lib_concepts instead of (C++20 && __cpp_lib_concepts). That
> would be fine, because __cpp_lib_concepts is only defined for C++20
> anyway. But we can change that later, not as part of this patch.

Sounds good.

> 
> >       using __base_ref = typename __traits_type::reference;
> > #endif
> > 
> 
> OK for trunk and gcc-11 branch, thanks.

Thanks a lot, committed!

> 
> We should also look into whether backporting to gcc-10 is feasible. I
> think it should be, but I didn't try.

I bet it would be modulo some small LWG issue fixes that I may have not
yet backported to the 10 branch.  I'll take a look :)