diff mbox series

libstdc++: Give ranges::empty() a concrete return type (PR 93978)

Message ID 20200305160247.2705537-1-ppalka@redhat.com
State New
Headers show
Series libstdc++: Give ranges::empty() a concrete return type (PR 93978) | expand

Commit Message

Patrick Palka March 5, 2020, 4:02 p.m. UTC
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
expression, it has type bool."

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.

libstdc++-v3/ChangeLog:

	PR libstdc++/93978
	* include/bits/range_access.h (__cust_access::_Empty::operator()):
	Declare return type to be bool instead of auto.
	* testsuite/std/ranges/adaptors/93978.cc: New test.
---
 libstdc++-v3/include/bits/range_access.h      |  2 +-
 .../testsuite/std/ranges/adaptors/93978.cc    | 34 +++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 libstdc++-v3/testsuite/std/ranges/adaptors/93978.cc

Comments

Jonathan Wakely March 6, 2020, 10:18 a.m. UTC | #1
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.
Patrick Palka March 6, 2020, 2:04 p.m. UTC | #2
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 mbox series

Patch

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;
+}