public inbox for libstdc++-cvs@sourceware.org
help / color / mirror / Atom feed
* [gcc r10-9737] libstdc++: Don't initialize from *this inside some views [PR97600]
@ 2021-04-21 3:27 Patrick Palka
0 siblings, 0 replies; only message in thread
From: Patrick Palka @ 2021-04-21 3:27 UTC (permalink / raw)
To: gcc-cvs, libstdc++-cvs
https://gcc.gnu.org/g:3d6bba85e1dd6cb7e213a7e6c060b9c8a0a346e2
commit r10-9737-g3d6bba85e1dd6cb7e213a7e6c060b9c8a0a346e2
Author: Patrick Palka <ppalka@redhat.com>
Date: Fri Oct 30 20:33:19 2020 -0400
libstdc++: Don't initialize from *this inside some views [PR97600]
This works around a subtle issue where instantiating the begin()/end()
member of some views (as part of return type deduction) inadvertently
requires computing the satisfaction value of range<foo_view>.
This is problematic because the constraint range<foo_view> requires the
begin()/end() member to be callable. But it's not callable until we've
deduced its return type, so evaluation of range<foo_view> yields false
at this point. And if after both members are instantiated (and their
return types deduced) we evaluate range<foo_view> again, this time it
will yield true since the begin()/end() members are now both callable.
This makes the program ill-formed according to [temp.constr.atomic]/3:
If, at different points in the program, the satisfaction result is
different for identical atomic constraints and template arguments, the
program is ill-formed, no diagnostic required.
The views affected by this issue are those whose begin()/end() member
has a placeholder return type and that member initializes an _Iterator
or _Sentinel object from a reference to *this. The second condition is
relevant because it means explicit conversion functions are considered
during overload resolution (as per [over.match.copy], I think), and
therefore it causes g++ to check the constraints of the conversion
function view_interface<foo_view>::operator bool(). And this conversion
function's constraints indirectly require range<foo_view>.
This issue is observable on trunk only with basic_istream_view (as in
the testcase in the PR). But a pending patch that makes g++ memoize
constraint satisfaction values indefinitely (it currently invalidates
the satisfaction cache on various events) causes many existing tests for
the other affected views to fail, because range<foo_view> then remains
false for the whole compilation.
This patch works around this issue by adjusting the constructors of the
_Iterator and _Sentinel types of the affected views to take their
foo_view argument by pointer instead of by reference, so that g++ no
longer considers explicit conversion functions when resolving the
direct-initialization inside these views' begin()/end() members.
libstdc++-v3/ChangeLog:
PR libstdc++/97600
* include/std/ranges (basic_istream_view::begin): Initialize
_Iterator from 'this' instead of '*this'.
(basic_istream_view::_Iterator::_Iterator): Adjust constructor
accordingly.
(filter_view::_Iterator::_Iterator): Take a filter_view*
argument instead of a filter_view& argument.
(filter_view::_Sentinel::_Sentinel): Likewise.
(filter_view::begin): Initialize _Iterator from 'this' instead
of '*this'.
(filter_view::end): Likewise.
(transform_view::_Iterator::_Iterator): Take a _Parent* instead
of a _Parent&.
(filter_view::_Iterator::operator+): Adjust accordingly.
(filter_view::_Iterator::operator-): Likewise.
(filter_view::begin): Initialize _Iterator from 'this' instead
of '*this'.
(filter_view::end): Likewise.
(join_view::_Iterator): Take a _Parent* instead of a _Parent&.
(join_view::_Sentinel): Likewise.
(join_view::begin): Initialize _Iterator from 'this' instead of
'*this'.
(join_view::end): Initialize _Sentinel from 'this' instead of
'*this'.
(split_view::_OuterIter): Take a _Parent& instead of a _Parent*.
(split_view::begin): Initialize _OuterIter from 'this' instead
of '*this'.
(split_view::end): Likewise.
* testsuite/std/ranges/97600.cc: New test.
(cherry picked from commit afb8da7faa9dfe5a0d94ed45a373d74c076784ab)
Diff:
---
libstdc++-v3/include/std/ranges | 78 +++++++++++++++---------------
libstdc++-v3/testsuite/std/ranges/97600.cc | 32 ++++++++++++
2 files changed, 71 insertions(+), 39 deletions(-)
diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
index a8ec806235d..ad882042475 100644
--- a/libstdc++-v3/include/std/ranges
+++ b/libstdc++-v3/include/std/ranges
@@ -958,7 +958,7 @@ namespace views
{
if (_M_stream != nullptr)
*_M_stream >> _M_object;
- return _Iterator{*this};
+ return _Iterator{this};
}
constexpr default_sentinel_t
@@ -979,8 +979,8 @@ namespace views
_Iterator() = default;
constexpr explicit
- _Iterator(basic_istream_view& __parent) noexcept
- : _M_parent(std::__addressof(__parent))
+ _Iterator(basic_istream_view* __parent) noexcept
+ : _M_parent(__parent)
{ }
_Iterator(const _Iterator&) = delete;
@@ -1467,9 +1467,9 @@ namespace views
_Iterator() = default;
constexpr
- _Iterator(filter_view& __parent, _Vp_iter __current)
+ _Iterator(filter_view* __parent, _Vp_iter __current)
: _M_current(std::move(__current)),
- _M_parent(std::__addressof(__parent))
+ _M_parent(__parent)
{ }
constexpr _Vp_iter
@@ -1559,8 +1559,8 @@ namespace views
_Sentinel() = default;
constexpr explicit
- _Sentinel(filter_view& __parent)
- : _M_end(ranges::end(__parent._M_base))
+ _Sentinel(filter_view* __parent)
+ : _M_end(ranges::end(__parent->_M_base))
{ }
constexpr sentinel_t<_Vp>
@@ -1600,23 +1600,23 @@ namespace views
begin()
{
if (_M_cached_begin._M_has_value())
- return {*this, _M_cached_begin._M_get(_M_base)};
+ return {this, _M_cached_begin._M_get(_M_base)};
__glibcxx_assert(_M_pred.has_value());
auto __it = __detail::find_if(ranges::begin(_M_base),
ranges::end(_M_base),
std::ref(*_M_pred));
_M_cached_begin._M_set(_M_base, __it);
- return {*this, std::move(__it)};
+ return {this, std::move(__it)};
}
constexpr auto
end()
{
if constexpr (common_range<_Vp>)
- return _Iterator{*this, ranges::end(_M_base)};
+ return _Iterator{this, ranges::end(_M_base)};
else
- return _Sentinel{*this};
+ return _Sentinel{this};
}
};
@@ -1695,9 +1695,9 @@ namespace views
_Iterator() = default;
constexpr
- _Iterator(_Parent& __parent, _Base_iter __current)
+ _Iterator(_Parent* __parent, _Base_iter __current)
: _M_current(std::move(__current)),
- _M_parent(std::__addressof(__parent))
+ _M_parent(__parent)
{ }
constexpr
@@ -1810,17 +1810,17 @@ namespace views
friend constexpr _Iterator
operator+(_Iterator __i, difference_type __n)
requires random_access_range<_Base>
- { return {*__i._M_parent, __i._M_current + __n}; }
+ { return {__i._M_parent, __i._M_current + __n}; }
friend constexpr _Iterator
operator+(difference_type __n, _Iterator __i)
requires random_access_range<_Base>
- { return {*__i._M_parent, __i._M_current + __n}; }
+ { return {__i._M_parent, __i._M_current + __n}; }
friend constexpr _Iterator
operator-(_Iterator __i, difference_type __n)
requires random_access_range<_Base>
- { return {*__i._M_parent, __i._M_current - __n}; }
+ { return {__i._M_parent, __i._M_current - __n}; }
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// 3483. transform_view::iterator's difference is overconstrained
@@ -1931,13 +1931,13 @@ namespace views
constexpr _Iterator<false>
begin()
- { return _Iterator<false>{*this, ranges::begin(_M_base)}; }
+ { return _Iterator<false>{this, ranges::begin(_M_base)}; }
constexpr _Iterator<true>
begin() const
requires range<const _Vp>
&& regular_invocable<const _Fp&, range_reference_t<const _Vp>>
- { return _Iterator<true>{*this, ranges::begin(_M_base)}; }
+ { return _Iterator<true>{this, ranges::begin(_M_base)}; }
constexpr _Sentinel<false>
end()
@@ -1945,7 +1945,7 @@ namespace views
constexpr _Iterator<false>
end() requires common_range<_Vp>
- { return _Iterator<false>{*this, ranges::end(_M_base)}; }
+ { return _Iterator<false>{this, ranges::end(_M_base)}; }
constexpr _Sentinel<true>
end() const
@@ -1957,7 +1957,7 @@ namespace views
end() const
requires common_range<const _Vp>
&& regular_invocable<const _Fp&, range_reference_t<const _Vp>>
- { return _Iterator<true>{*this, ranges::end(_M_base)}; }
+ { return _Iterator<true>{this, ranges::end(_M_base)}; }
constexpr auto
size() requires sized_range<_Vp>
@@ -2514,9 +2514,9 @@ namespace views
_Iterator() = default;
constexpr
- _Iterator(_Parent& __parent, _Outer_iter __outer)
+ _Iterator(_Parent* __parent, _Outer_iter __outer)
: _M_outer(std::move(__outer)),
- _M_parent(std::__addressof(__parent))
+ _M_parent(__parent)
{ _M_satisfy(); }
constexpr
@@ -2638,8 +2638,8 @@ namespace views
_Sentinel() = default;
constexpr explicit
- _Sentinel(_Parent& __parent)
- : _M_end(ranges::end(__parent._M_base))
+ _Sentinel(_Parent* __parent)
+ : _M_end(ranges::end(__parent->_M_base))
{ }
constexpr
@@ -2687,7 +2687,7 @@ namespace views
constexpr bool __use_const
= (__detail::__simple_view<_Vp>
&& is_reference_v<range_reference_t<_Vp>>);
- return _Iterator<__use_const>{*this, ranges::begin(_M_base)};
+ return _Iterator<__use_const>{this, ranges::begin(_M_base)};
}
constexpr auto
@@ -2695,7 +2695,7 @@ namespace views
requires input_range<const _Vp>
&& is_reference_v<range_reference_t<const _Vp>>
{
- return _Iterator<true>{*this, ranges::begin(_M_base)};
+ return _Iterator<true>{this, ranges::begin(_M_base)};
}
constexpr auto
@@ -2704,10 +2704,10 @@ namespace views
if constexpr (forward_range<_Vp> && is_reference_v<_InnerRange>
&& forward_range<_InnerRange>
&& common_range<_Vp> && common_range<_InnerRange>)
- return _Iterator<__detail::__simple_view<_Vp>>{*this,
+ return _Iterator<__detail::__simple_view<_Vp>>{this,
ranges::end(_M_base)};
else
- return _Sentinel<__detail::__simple_view<_Vp>>{*this};
+ return _Sentinel<__detail::__simple_view<_Vp>>{this};
}
constexpr auto
@@ -2720,9 +2720,9 @@ namespace views
&& forward_range<range_reference_t<const _Vp>>
&& common_range<const _Vp>
&& common_range<range_reference_t<const _Vp>>)
- return _Iterator<true>{*this, ranges::end(_M_base)};
+ return _Iterator<true>{this, ranges::end(_M_base)};
else
- return _Sentinel<true>{*this};
+ return _Sentinel<true>{this};
}
};
@@ -2841,14 +2841,14 @@ namespace views
_OuterIter() = default;
constexpr explicit
- _OuterIter(_Parent& __parent) requires (!forward_range<_Base>)
- : _M_parent(std::__addressof(__parent))
+ _OuterIter(_Parent* __parent) requires (!forward_range<_Base>)
+ : _M_parent(__parent)
{ }
constexpr
- _OuterIter(_Parent& __parent, iterator_t<_Base> __current)
+ _OuterIter(_Parent* __parent, iterator_t<_Base> __current)
requires forward_range<_Base>
- : _M_parent(std::__addressof(__parent)),
+ : _M_parent(__parent),
_M_current(std::move(__current))
{ }
@@ -3074,25 +3074,25 @@ namespace views
{
if constexpr (forward_range<_Vp>)
return _OuterIter<__detail::__simple_view<_Vp>>{
- *this, ranges::begin(_M_base)};
+ this, ranges::begin(_M_base)};
else
{
_M_current = ranges::begin(_M_base);
- return _OuterIter<false>{*this};
+ return _OuterIter<false>{this};
}
}
constexpr auto
begin() const requires forward_range<_Vp> && forward_range<const _Vp>
{
- return _OuterIter<true>{*this, ranges::begin(_M_base)};
+ return _OuterIter<true>{this, ranges::begin(_M_base)};
}
constexpr auto
end() requires forward_range<_Vp> && common_range<_Vp>
{
return _OuterIter<__detail::__simple_view<_Vp>>{
- *this, ranges::end(_M_base)};
+ this, ranges::end(_M_base)};
}
constexpr auto
@@ -3101,7 +3101,7 @@ namespace views
if constexpr (forward_range<_Vp>
&& forward_range<const _Vp>
&& common_range<const _Vp>)
- return _OuterIter<true>{*this, ranges::end(_M_base)};
+ return _OuterIter<true>{this, ranges::end(_M_base)};
else
return default_sentinel;
}
diff --git a/libstdc++-v3/testsuite/std/ranges/97600.cc b/libstdc++-v3/testsuite/std/ranges/97600.cc
new file mode 100644
index 00000000000..d992318259d
--- /dev/null
+++ b/libstdc++-v3/testsuite/std/ranges/97600.cc
@@ -0,0 +1,32 @@
+// 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 } }
+
+// PR libstdc++/97600
+
+#include <sstream>
+#include <ranges>
+
+void
+test01()
+{
+ std::ranges::basic_istream_view<int, char, std::char_traits<char>> v;
+ v.begin();
+ static_assert(std::ranges::range<decltype(v)>);
+}
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2021-04-21 3:27 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-21 3:27 [gcc r10-9737] libstdc++: Don't initialize from *this inside some views [PR97600] Patrick Palka
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).