Message ID | 20200501120310.GA498115@redhat.com |
---|---|
State | New |
Headers | show |
Series | libstdc++: Replace deduced return type in ranges::iter_move (PR 92894) | expand |
On 01/05/20 13:03 +0100, Jonathan Wakely wrote: >The deduced return type causes the instantiation of the function body, >which can then require the instantiation of std::projected::operator* >which is intentionally not defined. > >This patch uses a helper trait to define the return type, so that the >function body doesn't need to be instantiated. That helper trait can >then also be used in other places that currently check the return type >of ranges::iter_move (iter_rvalue_reference_t and indirectly_readable). > >2020-05-01 Jonathan Wakely <jwakely@redhat.com> > Patrick Palka <ppalka@redhat.com> > > PR libstdc++/92894 > * include/bits/iterator_concepts.h (ranges::__cust_imove::_IMove): > Add trait to determine return type and an alias for it. > (ranges::__cust_imove::_IMove::operator()): Use __result instead of > deduced return type. > (iter_rvalue_reference_t): Use _IMove::__type instead of checking > the result of ranges::iter_move. > (__detail::__indirectly_readable_impl): Use iter_rvalue_reference_t > instead of checking the result of ranges::iter_move. > * testsuite/24_iterators/indirect_callable/92894.cc: New test. > >Patrick, I prefer this to the patch you added to the bug. Avoiding >doing overload resolution on ranges::iter_move(x) seems worthwhile. > >What do you think? > >I *think* the changes to __indirectly_readable_impl are equivalent. As >far as I can tell, the point of the original { *in } >and { ranges::iter_move(in) } constraints are to check that const In >and In do the same thing. We could leave the { *in } on, but for >consistency it seems better to change both. Here's a slightly improved patch which I'm committing now. Diffs from the previous patch are removing the useless parameter list from the __indirectly_readable_impl concept: @@ -460,7 +461,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION using __iter_concept = typename __iter_concept_impl<_Iter>::type; template<typename _In> - concept __indirectly_readable_impl = requires(const _In __in) + concept __indirectly_readable_impl = requires { typename iter_value_t<_In>; typename iter_reference_t<_In>; Fixing 24_iterators/indirect_callable/92894.cc so it actually passes, and adding 24_iterators/customization_points/92894.cc to verify that iter_move is fixed (which the other tests don't do because indirectly_readable no longer uses ranges::iter_move). Tested powerpc64le-linux, committed to master.
On 01/05/20 14:28 +0100, Jonathan Wakely wrote: >On 01/05/20 13:03 +0100, Jonathan Wakely wrote: >>The deduced return type causes the instantiation of the function body, >>which can then require the instantiation of std::projected::operator* >>which is intentionally not defined. >> >>This patch uses a helper trait to define the return type, so that the >>function body doesn't need to be instantiated. That helper trait can >>then also be used in other places that currently check the return type >>of ranges::iter_move (iter_rvalue_reference_t and indirectly_readable). >> >>2020-05-01 Jonathan Wakely <jwakely@redhat.com> >> Patrick Palka <ppalka@redhat.com> >> >> PR libstdc++/92894 >> * include/bits/iterator_concepts.h (ranges::__cust_imove::_IMove): >> Add trait to determine return type and an alias for it. >> (ranges::__cust_imove::_IMove::operator()): Use __result instead of >> deduced return type. >> (iter_rvalue_reference_t): Use _IMove::__type instead of checking >> the result of ranges::iter_move. >> (__detail::__indirectly_readable_impl): Use iter_rvalue_reference_t >> instead of checking the result of ranges::iter_move. >> * testsuite/24_iterators/indirect_callable/92894.cc: New test. >> >>Patrick, I prefer this to the patch you added to the bug. Avoiding >>doing overload resolution on ranges::iter_move(x) seems worthwhile. >> >>What do you think? >> >>I *think* the changes to __indirectly_readable_impl are equivalent. As >>far as I can tell, the point of the original { *in } >>and { ranges::iter_move(in) } constraints are to check that const In >>and In do the same thing. We could leave the { *in } on, but for >>consistency it seems better to change both. > >Here's a slightly improved patch which I'm committing now. > >Diffs from the previous patch are removing the useless parameter list >from the __indirectly_readable_impl concept: > >@@ -460,7 +461,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > using __iter_concept = typename __iter_concept_impl<_Iter>::type; > > template<typename _In> >- concept __indirectly_readable_impl = requires(const _In __in) >+ concept __indirectly_readable_impl = requires > { > typename iter_value_t<_In>; > typename iter_reference_t<_In>; > >Fixing 24_iterators/indirect_callable/92894.cc so it actually passes, >and adding 24_iterators/customization_points/92894.cc to verify that >iter_move is fixed (which the other tests don't do because >indirectly_readable no longer uses ranges::iter_move). This is the patch I'm testing for the gcc-10 branch (if it gets RM approval). It doesn't do the compile-time optimization for iter_rvalue_reference_t and __indirectly_readable_impl, it just fixes the bug in ranges::iter_move. The optimization isn't required for correctness.
diff --git a/libstdc++-v3/include/bits/iterator_concepts.h b/libstdc++-v3/include/bits/iterator_concepts.h index b598532089e..d84e8639ea8 100644 --- a/libstdc++-v3/include/bits/iterator_concepts.h +++ b/libstdc++-v3/include/bits/iterator_concepts.h @@ -89,6 +89,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION struct _IMove { private: + template<typename _Tp> + struct __result + { using type = iter_reference_t<_Tp>; }; + + template<typename _Tp> + requires __adl_imove<_Tp> + struct __result<_Tp> + { using type = decltype(iter_move(std::declval<_Tp>())); }; + + template<typename _Tp> + requires (!__adl_imove<_Tp>) + && is_lvalue_reference_v<iter_reference_t<_Tp>> + struct __result<_Tp> + { using type = remove_reference_t<iter_reference_t<_Tp>>&&; }; + template<typename _Tp> static constexpr bool _S_noexcept() @@ -100,16 +115,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } public: - template<typename _Tp> - requires __adl_imove<_Tp> || requires(_Tp& __e) { *__e; } - constexpr decltype(auto) + // The result type of iter_move(std::declval<_Tp>()) + template<std::__detail::__dereferenceable _Tp> + using __type = typename __result<_Tp>::type; + + template<std::__detail::__dereferenceable _Tp> + constexpr __type<_Tp> operator()(_Tp&& __e) const noexcept(_S_noexcept<_Tp>()) { if constexpr (__adl_imove<_Tp>) return iter_move(static_cast<_Tp&&>(__e)); - else if constexpr (is_reference_v<iter_reference_t<_Tp>>) - return std::move(*__e); + else if constexpr (is_lvalue_reference_v<iter_reference_t<_Tp>>) + return static_cast<__type<_Tp>>(*__e); else return *__e; } @@ -123,10 +141,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } // namespace ranges template<__detail::__dereferenceable _Tp> - requires requires(_Tp& __t) - { { ranges::iter_move(__t) } -> __detail::__can_reference; } + requires __detail::__can_reference<ranges::__cust_imove::_IMove::__type<_Tp&>> using iter_rvalue_reference_t - = decltype(ranges::iter_move(std::declval<_Tp&>())); + = ranges::__cust_imove::_IMove::__type<_Tp&>; template<typename> struct incrementable_traits { }; @@ -448,8 +465,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION typename iter_value_t<_In>; typename iter_reference_t<_In>; typename iter_rvalue_reference_t<_In>; - { *__in } -> same_as<iter_reference_t<_In>>; - { ranges::iter_move(__in) } -> same_as<iter_rvalue_reference_t<_In>>; + requires same_as<iter_reference_t<const _In>, + iter_reference_t<_In>>; + requires same_as<iter_rvalue_reference_t<const _In>, + iter_rvalue_reference_t<_In>>; } && common_reference_with<iter_reference_t<_In>&&, iter_value_t<_In>&> && common_reference_with<iter_reference_t<_In>&&, diff --git a/libstdc++-v3/testsuite/24_iterators/indirect_callable/92894.cc b/libstdc++-v3/testsuite/24_iterators/indirect_callable/92894.cc new file mode 100644 index 00000000000..78c4585923f --- /dev/null +++ b/libstdc++-v3/testsuite/24_iterators/indirect_callable/92894.cc @@ -0,0 +1,59 @@ +// Copyright (C) 2020 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 } } + +#include <iterator> + +using std::projected; +using std::identity; +using std::indirect_unary_predicate; + +template<typename T, + indirect_unary_predicate<projected<T*, identity>> Pred> + constexpr void + all_of(T*, Pred) + { } + +void +test01() +{ + // PR libstdc++/92894 + struct X { }; + X x; + all_of(&x, [](X&) { return false; }); +} + +using std::iterator_t; +using std::ranges::input_range; +using std::ranges::borrowed_range; + +template<input_range R, class Proj = identity, + indirect_unary_predicate<projected<iterator_t<R>, Proj>> Pred> + constexpr borrowed_iterator_t<R> + find_if(R&& r, Pred pred, Proj proj = {}) + { } + +void +test02() +{ + // PR 94241 + struct s { int m; }; + s r[] = { s{0}, s{1}, s{2}, s{3} }; + find_if(r, [](auto const) { return true; }); +}