Re: [PATCH] libstdc++: Simplify range adaptors' forwarding of bound args when possible

[Patrick Palka <ppalka@redhat.com>]

On Fri, 14 May 2021, Patrick Palka wrote:

> r11-8053 rewrote the range adaptor implementation in conformance with
> P2281, making partial application act like a SFINAE-friendly perfect
> forwarding call wrapper.  Making SFINAE-friendliness coexist with
> perfect forwarding here requires adding fallback deleted operator()
> overloads (as described in e.g. section 5.5 of wg21.link/p0847r6).  But
> unfortunately, as reported in PR100577, this necessary technique of
> using of deleted overloads regresses diagnostics for ill-formed calls to
> partially applied range adaptors in GCC.
> 
> Although GCC's diagnostics can arguably be improved here by having the
> compiler explain why the other candidates weren't viable when overload
> resolution selects a deleted candidate, we can also largely work around
> this on the library side (and achieve more concise diagnostics than by
> a frontend-side improvement alone) if we take advantage of the
> observation that not all range adaptors need perfect forwarding call
> wrapper semantics, in fact only views::split currently needs it, because
> all other range adaptors either take no extra arguments or only
> arguments that are expected to be freely/cheaply copyable, e.g. function
> objects and integer-like types.  (The discussion section of P2281 goes
> into detail about why views::split is special.)
> 
> To that end, this introduces opt-in flags for denoting a range adaptor
> as having "simple" extra arguments (in the case of a range adaptor
> non-closure) or having a "simple" call operator (in the case of a range
> adaptor closure).  These flags are then used to conditionally simplify
> the operator() for the generic _Partial and _Pipe class templates, down
> from needing three overloads thereof (including one defined as deleted)
> to just needing a single overload.  The end result is that diagnostic
> quality is restored for all adaptors except for views::split, and
> diagnostics for the adaptors are generally made more concise since
> there's only a single _Partial/_Pipe overload to diagnose instead of
> three of them.

Ping.

> 
> Tested on x86_64-pc-linux-gnu, does this look OK for trunk/11?
> 
> libstdc++-v3/ChangeLog:
> 
> 	PR libstdc++/100577
> 	* include/std/ranges (_RangeAdaptorClosure): Document
> 	_S_has_simple_call_op mechanism.
> 	(_RangeAdaptor): Document _S_has_simple_extra_args mechanism.
> 	(__closure_has_simple_call_op): New concept.
> 	(__adaptor_has_simple_extra_args): Likewise.
> 	(_Partial<_Adaptor, _Args...>): New partial specialization.
> 	(_Partial<_Adaptor, _Arg>): Likewise.
> 	(_Pipe<_Lhs, _Rhs>): Likewise.
> 	(views::_All::_S_has_simple_call_op): Define to true.
> 	(views::_Filter::_S_has_simple_extra_args): Likewise.
> 	(views::_Transform::_S_has_simple_extra_args): Likewise.
> 	(views::_Take::_S_has_simple_extra_args): Likewise.
> 	(views::_TakeWhile::_S_has_simple_extra_args): Likewise.
> 	(views::_Drop::_S_has_simple_extra_args): Likewise.
> 	(views::_DropWhile::_S_has_simple_extra_args): Likewise.
> 	(views::_Join::_S_has_simple_call_op): Likewise.
> 	(views::_Split): Document why we don't define
> 	_S_has_simple_extra_args to true for this adaptor.
> 	(views::_Common::_S_has_simple_call_op): Define to true.
> 	(views::_Reverse::_S_has_simple_call_op): Likewise.
> 	(views::_Elements::_S_has_simple_call_op): Likewise.
> 	* testsuite/std/ranges/adaptors/100577.cc: New test.
> ---
>  libstdc++-v3/include/std/ranges               | 119 +++++++++++++++++-
>  .../testsuite/std/ranges/adaptors/100577.cc   | 101 +++++++++++++++
>  2 files changed, 219 insertions(+), 1 deletion(-)
>  create mode 100644 libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
> 
> diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
> index 48100e9d7f2..0f69d4f0839 100644
> --- a/libstdc++-v3/include/std/ranges
> +++ b/libstdc++-v3/include/std/ranges
> @@ -756,6 +756,10 @@ namespace views::__adaptor
>      struct _Pipe;
>  
>    // The base class of every range adaptor closure.
> +  //
> +  // The derived class should define the optional static data member
> +  // _S_has_simple_call_op to true if the behavior of this adaptor is
> +  // independent of the constness/value category of the adaptor object.
>    struct _RangeAdaptorClosure
>    {
>      // range | adaptor is equivalent to adaptor(range).
> @@ -781,6 +785,10 @@ namespace views::__adaptor
>    // The static data member _Derived::_S_arity must contain the total number of
>    // arguments that the adaptor takes, and the class _Derived must introduce
>    // _RangeAdaptor::operator() into the class scope via a using-declaration.
> +  //
> +  // The optional static data member _Derived::_S_has_simple_extra_args should
> +  // be defined to true if the behavior of this adaptor is independent of the
> +  // constness/value category of the extra arguments.
>    template<typename _Derived>
>      struct _RangeAdaptor
>      {
> @@ -795,6 +803,17 @@ namespace views::__adaptor
>  	}
>      };
>  
> +  // True if the range adaptor closure _Adaptor has a simple operator(), i.e.
> +  // one that's not overloaded according to constness or value category of the
> +  // _Adaptor object.
> +  template<typename _Adaptor>
> +    concept __closure_has_simple_call_op = _Adaptor::_S_has_simple_call_op;
> +
> +  // True if the behavior of the range adaptor non-closure _Adaptor is
> +  // independent of the value category of its extra arguments.
> +  template<typename _Adaptor>
> +    concept __adaptor_has_simple_extra_args = _Adaptor::_S_has_simple_extra_args;
> +
>    // A range adaptor closure that represents partial application of
>    // the range adaptor _Adaptor with arguments _Args.
>    template<typename _Adaptor, typename... _Args>
> @@ -808,7 +827,7 @@ namespace views::__adaptor
>        { }
>  
>        // Invoke _Adaptor with arguments __r, _M_args... according to the
> -      // value category of the range adaptor closure object.
> +      // value category of this _Partial object.
>        template<typename _Range>
>  	requires __adaptor_invocable<_Adaptor, _Range, const _Args&...>
>  	constexpr auto
> @@ -865,6 +884,59 @@ namespace views::__adaptor
>  	operator()(_Range&& __r) const && = delete;
>      };
>  
> +  // Partial specialization of the primary template for the case where the extra
> +  // arguments of the adaptor can always be safely forwarded by const reference.
> +  // This lets us get away with a single operator() overload, which makes
> +  // overload resolution failure diagnostics more concise.
> +  template<typename _Adaptor, typename... _Args>
> +    requires __adaptor_has_simple_extra_args<_Adaptor>
> +    struct _Partial<_Adaptor, _Args...> : _RangeAdaptorClosure
> +    {
> +      tuple<_Args...> _M_args;
> +
> +      constexpr
> +      _Partial(_Args... __args)
> +	: _M_args(std::move(__args)...)
> +      { }
> +
> +      // Invoke _Adaptor with arguments __r, const _M_args&... regardless
> +      // of the value category of this _Partial object.
> +      template<typename _Range>
> +	requires __adaptor_invocable<_Adaptor, _Range, const _Args&...>
> +	constexpr auto
> +	operator()(_Range&& __r) const
> +	{
> +	  auto __forwarder = [&__r] (const auto&... __args) {
> +	    return _Adaptor{}(std::forward<_Range>(__r), __args...);
> +	  };
> +	  return std::apply(__forwarder, _M_args);
> +	}
> +
> +      static constexpr bool _S_has_simple_call_op = true;
> +    };
> +
> +  // A lightweight specialization of the above template for the common case
> +  // where _Adaptor accepts a single extra argument.
> +  template<typename _Adaptor, typename _Arg>
> +    requires __adaptor_has_simple_extra_args<_Adaptor>
> +    struct _Partial<_Adaptor, _Arg> : _RangeAdaptorClosure
> +    {
> +      _Arg _M_arg;
> +
> +      constexpr
> +      _Partial(_Arg __arg)
> +	: _M_arg(std::move(__arg))
> +      { }
> +
> +      template<typename _Range>
> +	requires __adaptor_invocable<_Adaptor, _Range, const _Arg&>
> +	constexpr auto
> +	operator()(_Range&& __r) const
> +	{ return _Adaptor{}(std::forward<_Range>(__r), _M_arg); }
> +
> +      static constexpr bool _S_has_simple_call_op = true;
> +    };
> +
>    template<typename _Lhs, typename _Rhs, typename _Range>
>      concept __pipe_invocable
>        = requires { std::declval<_Rhs>()(std::declval<_Lhs>()(std::declval<_Range>())); };
> @@ -900,6 +972,32 @@ namespace views::__adaptor
>  	constexpr auto
>  	operator()(_Range&& __r) const && = delete;
>      };
> +
> +  // A partial specialization of the above primary template for the case where
> +  // both adaptor operands have a simple operator().  This in turn lets us
> +  // implement composition using a single simple operator(), which makes
> +  // overload resolution failure diagnostics more concise.
> +  template<typename _Lhs, typename _Rhs>
> +    requires __closure_has_simple_call_op<_Lhs>
> +      && __closure_has_simple_call_op<_Rhs>
> +    struct _Pipe<_Lhs, _Rhs> : _RangeAdaptorClosure
> +    {
> +      [[no_unique_address]] _Lhs _M_lhs;
> +      [[no_unique_address]] _Rhs _M_rhs;
> +
> +      constexpr
> +      _Pipe(_Lhs __lhs, _Rhs __rhs)
> +	: _M_lhs(std::move(__lhs)), _M_rhs(std::move(__rhs))
> +      { }
> +
> +      template<typename _Range>
> +	requires __pipe_invocable<const _Lhs&, const _Rhs&, _Range>
> +	constexpr auto
> +	operator()(_Range&& __r) const
> +	{ return _M_rhs(_M_lhs(std::forward<_Range>(__r))); }
> +
> +      static constexpr bool _S_has_simple_call_op = true;
> +    };
>  } // namespace views::__adaptor
>  
>    template<range _Range> requires is_object_v<_Range>
> @@ -981,6 +1079,8 @@ namespace views::__adaptor
>  	  else
>  	    return subrange{std::forward<_Range>(__r)};
>  	}
> +
> +      static constexpr bool _S_has_simple_call_op = true;
>      };
>  
>      inline constexpr _All all;
> @@ -1353,6 +1453,7 @@ namespace views::__adaptor
>  
>        using _RangeAdaptor<_Filter>::operator();
>        static constexpr int _S_arity = 2;
> +      static constexpr bool _S_has_simple_extra_args = true;
>      };
>  
>      inline constexpr _Filter filter;
> @@ -1727,6 +1828,7 @@ namespace views::__adaptor
>  
>        using _RangeAdaptor<_Transform>::operator();
>        static constexpr int _S_arity = 2;
> +      static constexpr bool _S_has_simple_extra_args = true;
>      };
>  
>      inline constexpr _Transform transform;
> @@ -1907,6 +2009,7 @@ namespace views::__adaptor
>  
>        using _RangeAdaptor<_Take>::operator();
>        static constexpr int _S_arity = 2;
> +      static constexpr bool _S_has_simple_extra_args = true;
>      };
>  
>      inline constexpr _Take take;
> @@ -2026,6 +2129,7 @@ namespace views::__adaptor
>  
>        using _RangeAdaptor<_TakeWhile>::operator();
>        static constexpr int _S_arity = 2;
> +      static constexpr bool _S_has_simple_extra_args = true;
>      };
>  
>      inline constexpr _TakeWhile take_while;
> @@ -2145,6 +2249,7 @@ namespace views::__adaptor
>  
>        using _RangeAdaptor<_Drop>::operator();
>        static constexpr int _S_arity = 2;
> +      static constexpr bool _S_has_simple_extra_args = true;
>      };
>  
>      inline constexpr _Drop drop;
> @@ -2228,6 +2333,7 @@ namespace views::__adaptor
>  
>        using _RangeAdaptor<_DropWhile>::operator();
>        static constexpr int _S_arity = 2;
> +      static constexpr bool _S_has_simple_extra_args = true;
>      };
>  
>      inline constexpr _DropWhile drop_while;
> @@ -2645,6 +2751,8 @@ namespace views::__adaptor
>  	  // 3474. Nesting join_views is broken because of CTAD
>  	  return join_view<all_t<_Range>>{std::forward<_Range>(__r)};
>  	}
> +
> +      static constexpr bool _S_has_simple_call_op = true;
>      };
>  
>      inline constexpr _Join join;
> @@ -3078,6 +3186,9 @@ namespace views::__adaptor
>  
>        using _RangeAdaptor<_Split>::operator();
>        static constexpr int _S_arity = 2;
> +      // The second argument of views::split is _not_ simple -- it can be a
> +      // non-view range, the value category of which affects whether the call is
> +      // well-formed.  So we must not define _S_has_simple_extra_args to true.
>      };
>  
>      inline constexpr _Split split;
> @@ -3217,6 +3328,8 @@ namespace views::__adaptor
>  	  else
>  	    return common_view{std::forward<_Range>(__r)};
>  	}
> +
> +      static constexpr bool _S_has_simple_call_op = true;
>      };
>  
>      inline constexpr _Common common;
> @@ -3346,6 +3459,8 @@ namespace views::__adaptor
>  	  else
>  	    return reverse_view{std::forward<_Range>(__r)};
>  	}
> +
> +      static constexpr bool _S_has_simple_call_op = true;
>      };
>  
>      inline constexpr _Reverse reverse;
> @@ -3723,6 +3838,8 @@ namespace views::__adaptor
>  	  {
>  	    return elements_view<all_t<_Range>, _Nm>{std::forward<_Range>(__r)};
>  	  }
> +
> +	static constexpr bool _S_has_simple_call_op = true;
>        };
>  
>      template<size_t _Nm>
> diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
> new file mode 100644
> index 00000000000..719ba15d947
> --- /dev/null
> +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/100577.cc
> @@ -0,0 +1,101 @@
> +// Copyright (C) 2021 Free Software Foundation, Inc.
> +//
> +// This file is part of the GNU ISO C++ Library.  This library is free
> +// software; you can redistribute it and/or modify it under the
> +// terms of the GNU General Public License as published by the
> +// Free Software Foundation; either version 3, or (at your option)
> +// any later version.
> +
> +// This library is distributed in the hope that it will be useful,
> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +// GNU General Public License for more details.
> +
> +// You should have received a copy of the GNU General Public License along
> +// with this library; see the file COPYING3.  If not see
> +// <http://www.gnu.org/licenses/>.
> +
> +// { dg-options "-std=gnu++2a" }
> +// { dg-do compile { target c++2a } }
> +
> +// PR libstdc++/100577
> +
> +#include <ranges>
> +
> +namespace ranges = std::ranges;
> +namespace views = std::ranges::views;
> +
> +void
> +test01()
> +{
> +  // Verify all multi-argument adaptors except for views::split are denoted
> +  // to have simple extra arguments.
> +  using views::__adaptor::__adaptor_has_simple_extra_args;
> +  static_assert(__adaptor_has_simple_extra_args<decltype(views::transform)>);
> +  static_assert(__adaptor_has_simple_extra_args<decltype(views::filter)>);
> +  static_assert(__adaptor_has_simple_extra_args<decltype(views::drop)>);
> +  static_assert(__adaptor_has_simple_extra_args<decltype(views::take)>);
> +  static_assert(__adaptor_has_simple_extra_args<decltype(views::take_while)>);
> +  static_assert(__adaptor_has_simple_extra_args<decltype(views::drop_while)>);
> +  static_assert(!__adaptor_has_simple_extra_args<decltype(views::split)>);
> +
> +  // Verify all adaptor closures except for views::split(foo) have a simple
> +  // operator().
> +  using views::__adaptor::__closure_has_simple_call_op;
> +  __closure_has_simple_call_op auto a00 = views::all;
> +  __closure_has_simple_call_op auto a01 = views::transform(std::identity{});
> +  __closure_has_simple_call_op auto a02 = views::filter(std::identity{});
> +  __closure_has_simple_call_op auto a03 = views::drop(42);
> +  __closure_has_simple_call_op auto a04 = views::take(42);
> +  __closure_has_simple_call_op auto a05 = views::take_while(std::identity{});
> +  __closure_has_simple_call_op auto a06 = views::drop_while(std::identity{});
> +  __closure_has_simple_call_op auto a07 = views::join;
> +  __closure_has_simple_call_op auto a08 = views::common;
> +  __closure_has_simple_call_op auto a09 = views::reverse;
> +  __closure_has_simple_call_op auto a10 = views::keys;
> +  // Verify composition of simple closures is simple.
> +  __closure_has_simple_call_op auto b
> +    = (a00 | a01) | (a02 | a03) | (a04 | a05 | a06) | (a07 | a08 | a09 | a10);
> +
> +  // Verify views::split is the exception.
> +  auto a11 = views::split(' ');
> +  static_assert(!__closure_has_simple_call_op<decltype(a11)>);
> +  static_assert(!__closure_has_simple_call_op<decltype(a11 | a00)>);
> +  static_assert(!__closure_has_simple_call_op<decltype(a00 | a11)>);
> +}
> +
> +void
> +test02()
> +{
> +  // Range adaptor closures with a simple operator() aren't implemented using a
> +  // fallback deleted overload, so when a call is ill-formed overload resolution
> +  // fails.
> +  extern int x[10];
> +  auto badarg = nullptr;
> +  views::transform(badarg)(x); // { dg-error "no match" }
> +  views::filter(badarg)(x); // { dg-error "no match" }
> +  views::take(badarg)(x); // { dg-error "no match" }
> +  views::drop(badarg)(x); // { dg-error "no match" }
> +  views::take_while(badarg)(x); // { dg-error "no match" }
> +  views::drop_while(badarg)(x); // { dg-error "no match" }
> +
> +  (views::transform(badarg) | views::all)(x); // { dg-error "no match" }
> +  (views::filter(badarg) | views::all)(x); // { dg-error "no match" }
> +  (views::take(badarg) | views::all)(x); // { dg-error "no match" }
> +  (views::drop(badarg) | views::all)(x); // { dg-error "no match" }
> +  (views::take_while(badarg) | views::all)(x); // { dg-error "no match" }
> +  (views::drop_while(badarg) | views::all)(x); // { dg-error "no match" }
> +
> +  // In practice, range adaptor closures with non-simple operator() are
> +  // implemented using a fallback deleted overload, so when a call is
> +  // ill-formed overload resolution succeeds but selects the deleted overload
> +  // (but only when the closure is invoked as an rvalue).
> +  views::split(badarg)(x); // { dg-error "deleted function" }
> +  (views::split(badarg) | views::all)(x); // { dg-error "deleted function" }
> +  auto a0 = views::split(badarg);
> +  a0(x); // { dg-error "no match" };
> +  auto a1 = a0 | views::all;
> +  a1(x); // { dg-error "no match" }
> +}
> +
> +// { dg-prune-output "in requirements" }
> -- 
> 2.31.1.606.gdf6c4f722c
> 
>