Message ID | 20200305160247.2705537-1-ppalka@redhat.com |
---|---|
State | New |
Headers | show |
Series | libstdc++: Give ranges::empty() a concrete return type (PR 93978) | expand |
On 05/03/20 11:02 -0500, Patrick Palka wrote: >This works around PR 93978 by avoiding having to instantiate ranges::empty() >when checking the constraints of view_interface::operator bool(). When >ranges::empty() has an auto return type, then we must instantiate it in order to >determine whether the requires expression { ranges::empty(_M_derived()); } is >well-formed. But this means instantiating view_interface::empty() and hence >view_interface::_M_derived(), all before we've yet deduced the return type of >join_view::end(). (The reason view_interface::operator bool() is needed in >join_view::end() in the first place is because in this function we perform >direct initialization of join_view::_Sentinel from a join_view, and so we try to >find a conversion sequence from the latter to the former that goes through this >conversion operator.) > >Giving ranges::empty() a concrete return type of bool should be safe according >to [ranges.prim.empty]/4 which says "whenever ranges::empty(E) is a valid N.B. [range.prim.empty] in the commit log here please, not "ranges." >expression, it has type bool." Right, I don't know why I didn't make it just return bool anyway. Probably because I was copy&pasting _Begin for each of those other range access CPOs. This way we've become aware of the danger of using deduced return types, and so can audit the other cases. >This fixes the test case in PR 93978 when compiling without -Wall, but with -Wall >the test case still fails due to the issue described in PR c++/94038, I think. >I still don't quite understand why the test case doesn't fail without -O. Still a small improvement. OK for master, thanks.
On Fri, 6 Mar 2020, Jonathan Wakely wrote: > On 05/03/20 11:02 -0500, Patrick Palka wrote: > > This works around PR 93978 by avoiding having to instantiate ranges::empty() > > when checking the constraints of view_interface::operator bool(). When > > ranges::empty() has an auto return type, then we must instantiate it in > > order to > > determine whether the requires expression { ranges::empty(_M_derived()); } > > is > > well-formed. But this means instantiating view_interface::empty() and hence > > view_interface::_M_derived(), all before we've yet deduced the return type > > of > > join_view::end(). (The reason view_interface::operator bool() is needed in > > join_view::end() in the first place is because in this function we perform > > direct initialization of join_view::_Sentinel from a join_view, and so we > > try to > > find a conversion sequence from the latter to the former that goes through > > this > > conversion operator.) > > > > Giving ranges::empty() a concrete return type of bool should be safe > > according > > to [ranges.prim.empty]/4 which says "whenever ranges::empty(E) is a valid > > N.B. [range.prim.empty] in the commit log here please, not "ranges." Fixed. > > > expression, it has type bool." > > Right, I don't know why I didn't make it just return bool anyway. > Probably because I was copy&pasting _Begin for each of those other > range access CPOs. > > This way we've become aware of the danger of using deduced return > types, and so can audit the other cases. It seems that all of the other range access CPOs (since they are each defined with a deduced return type) have some form of this issue: using them as an unevaluated operand may cause more templates to get instantiated than when using the corresponding expression-equivalent expression. In this specific case, checking the constraint requires { ranges::empty(_M_derived()); } requires instantiating the body of _Derived::empty(), whereas checking the expression-equivalent constraint requires { _M_derived().empty(); } doesn't. If this discrepancy is important, then it seems we need to avoid using a deduced return type in the definitions of each of these CPOs. Is this worth doing? > > > This fixes the test case in PR 93978 when compiling without -Wall, but with > > -Wall > > the test case still fails due to the issue described in PR c++/94038, I > > think. > > I still don't quite understand why the test case doesn't fail without -O. > > Still a small improvement. > > OK for master, thanks.
diff --git a/libstdc++-v3/include/bits/range_access.h b/libstdc++-v3/include/bits/range_access.h index c814694623c..eab8bb9721b 100644 --- a/libstdc++-v3/include/bits/range_access.h +++ b/libstdc++-v3/include/bits/range_access.h @@ -804,7 +804,7 @@ namespace ranges template<typename _Tp> requires __member_empty<_Tp> || __size0_empty<_Tp> || __eq_iter_empty<_Tp> - constexpr auto + constexpr bool operator()(_Tp&& __e) const noexcept(_S_noexcept<_Tp>()) { if constexpr (__member_empty<_Tp>) diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/93978.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/93978.cc new file mode 100644 index 00000000000..62b5f5f9dac --- /dev/null +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/93978.cc @@ -0,0 +1,34 @@ +// 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 -O" } +// { dg-do compile { target c++2a } } + +#include <ranges> +#include <vector> + +namespace ranges = std::ranges; +namespace views = std::views; + +void +test() +{ + std::vector<std::string> x = {""}; + auto i = std::counted_iterator(x.begin(), 1); + auto r = ranges::subrange{i, std::default_sentinel}; + auto v = r | views::join; +}