From: Patrick Palka Date: Sat, 31 Oct 2020 00:33:19 +0000 (-0400) Subject: libstdc++: Don't initialize from *this inside some views [PR97600] X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=afb8da7faa9dfe5a0d94ed45a373d74c076784ab;p=gcc.git 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. This is problematic because the constraint range requires the begin()/end() member to be callable. But it's not callable until we've deduced its return type, so evaluation of range yields false at this point. And if after both members are instantiated (and their return types deduced) we evaluate range 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::operator bool(). And this conversion function's constraints indirectly require range. 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 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. --- diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges index bc7bb05b005..14d2a11f7fb 100644 --- a/libstdc++-v3/include/std/ranges +++ b/libstdc++-v3/include/std/ranges @@ -636,7 +636,7 @@ namespace views { if (_M_stream != nullptr) *_M_stream >> _M_object; - return _Iterator{*this}; + return _Iterator{this}; } constexpr default_sentinel_t @@ -657,8 +657,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; @@ -1147,9 +1147,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 @@ -1239,8 +1239,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> @@ -1280,23 +1280,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}; } }; @@ -1375,9 +1375,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 @@ -1490,17 +1490,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 @@ -1611,13 +1611,13 @@ namespace views constexpr _Iterator begin() - { return _Iterator{*this, ranges::begin(_M_base)}; } + { return _Iterator{this, ranges::begin(_M_base)}; } constexpr _Iterator begin() const requires range && regular_invocable> - { return _Iterator{*this, ranges::begin(_M_base)}; } + { return _Iterator{this, ranges::begin(_M_base)}; } constexpr _Sentinel end() @@ -1625,7 +1625,7 @@ namespace views constexpr _Iterator end() requires common_range<_Vp> - { return _Iterator{*this, ranges::end(_M_base)}; } + { return _Iterator{this, ranges::end(_M_base)}; } constexpr _Sentinel end() const @@ -1637,7 +1637,7 @@ namespace views end() const requires common_range && regular_invocable> - { return _Iterator{*this, ranges::end(_M_base)}; } + { return _Iterator{this, ranges::end(_M_base)}; } constexpr auto size() requires sized_range<_Vp> @@ -2193,9 +2193,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 @@ -2315,8 +2315,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 @@ -2363,7 +2363,7 @@ namespace views constexpr bool __use_const = (__detail::__simple_view<_Vp> && is_reference_v>); - return _Iterator<__use_const>{*this, ranges::begin(_M_base)}; + return _Iterator<__use_const>{this, ranges::begin(_M_base)}; } constexpr auto @@ -2371,7 +2371,7 @@ namespace views requires input_range && is_reference_v> { - return _Iterator{*this, ranges::begin(_M_base)}; + return _Iterator{this, ranges::begin(_M_base)}; } constexpr auto @@ -2380,10 +2380,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 @@ -2396,9 +2396,9 @@ namespace views && forward_range> && common_range && common_range>) - return _Iterator{*this, ranges::end(_M_base)}; + return _Iterator{this, ranges::end(_M_base)}; else - return _Sentinel{*this}; + return _Sentinel{this}; } }; @@ -2517,14 +2517,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)) { } @@ -2749,25 +2749,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{*this}; + return _OuterIter{this}; } } constexpr auto begin() const requires forward_range<_Vp> && forward_range { - return _OuterIter{*this, ranges::begin(_M_base)}; + return _OuterIter{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 @@ -2776,7 +2776,7 @@ namespace views if constexpr (forward_range<_Vp> && forward_range && common_range) - return _OuterIter{*this, ranges::end(_M_base)}; + return _OuterIter{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 +// . + +// { dg-options "-std=gnu++2a" } +// { dg-do compile { target c++2a } } + +// PR libstdc++/97600 + +#include +#include + +void +test01() +{ + std::ranges::basic_istream_view> v; + v.begin(); + static_assert(std::ranges::range); +}