[committed] libstdc++: Fix invalid noexcept-specifier (PR 94117)

Message ID 20200310114027.GA3370377@redhat.com
State New
Headers show
Series
  • [committed] libstdc++: Fix invalid noexcept-specifier (PR 94117)
Related show

Commit Message

Jonathan Wakely March 10, 2020, 11:40 a.m.
G++ fails to diagnose this non-dependent expression, but Clang doesn't
like it.

	PR c++/94117
	* include/std/ranges (ranges::transform_view::_Iterator::iter_move):
	Change expression in noexcept-specifier to match function body.

Tested x86_64-linux, committed to master.
commit c222eabcf8be0e3f644e4bd4c3316b40dba4b514
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Mar 10 10:50:40 2020 +0000

    libstdc++: Fix invalid noexcept-specifier (PR 94117)
    
    G++ fails to diagnose this non-dependent expression, but Clang doesn't
    like it.
    
            PR c++/94117
            * include/std/ranges (ranges::transform_view::_Iterator::iter_move):
            Change expression in noexcept-specifier to match function body.

Comments

Kees Cook via Gcc-patches March 10, 2020, 5:52 p.m. | #1
On 10/03/20 11:40 +0000, Jonathan Wakely wrote:
>G++ fails to diagnose this non-dependent expression, but Clang doesn't

>like it.

>

>	PR c++/94117

>	* include/std/ranges (ranges::transform_view::_Iterator::iter_move):

>	Change expression in noexcept-specifier to match function body.

>



This patch goes further and removes the __iter_move helper completely,
and the __iter_swap one, in transform_view.

It also does the same in split_view, and fixes a bug where the
noexcept-specifier was always false.

I've also added new _M_i_current() accessors (overloaded for const and
non-const) to return _M_i.__current(). Using this instead of
_M_i._M_current fixes a bug in inner-iterator::operator*() (which is
also present in the working draft).

Tested powerpc64le-linux, committed to master.
commit cf0c3a457319df1e8dc9321227162a7c57169a39
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Mar 10 17:45:45 2020 +0000

    libstdc++: Fix noexcept guarantees for ranges::split_view
    
    Also introduce the _M_i_current() accessors to solve the problem of
    access to the private member of _OuterIter from the iter_move and
    iter_swap overloads (which are only friends of _InnerIter not
    _OuterIter).
    
            * include/std/ranges (transform_view::_Iterator::__iter_move): Remove.
            (transform_view::_Iterator::operator*): Add noexcept-specifier.
            (transform_view::_Iterator::iter_move): Inline __iter_move body here.
            (split_view::_OuterIter::__current): Add noexcept.
            (split_view::_InnerIter::__iter_swap): Remove.
            (split_view::_InnerIter::__iter_move): Remove.
            (split_view::_InnerIter::_M_i_current): New accessors.
            (split_view::_InnerIter::__at_end): Use _M_i_current().
            (split_view::_InnerIter::operator*): Likewise.
            (split_view::_InnerIter::operator++): Likewise.
            (iter_move(const _InnerIter&)): Likewise.
            (iter_swap(const _InnerIter&, const _InnerIter&)): Likewise.
            * testsuite/std/ranges/adaptors/split.cc: Check noexcept-specifier
            for iter_move and iter_swap on split_view's inner iterator.

diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
index 292132db990..4dc7342e2f7 100644
--- a/libstdc++-v3/include/std/ranges
+++ b/libstdc++-v3/include/std/ranges
@@ -1679,17 +1679,6 @@ namespace views
 	      return input_iterator_tag{};
 	  }
 
-	  static constexpr decltype(auto)
-	  __iter_move(const _Iterator& __i = {})
-	    noexcept(noexcept(std::__invoke(*__i._M_parent->_M_fun,
-					    *__i._M_current)))
-	  {
-	    if constexpr (is_lvalue_reference_v<decltype(*__i)>)
-	      return std::move(*__i);
-	    else
-	      return *__i;
-	  }
-
 	  using _Base_iter = iterator_t<_Base>;
 
 	  _Base_iter _M_current = _Base_iter();
@@ -1728,6 +1717,7 @@ namespace views
 
 	  constexpr decltype(auto)
 	  operator*() const
+	    noexcept(noexcept(std::__invoke(*_M_parent->_M_fun, *_M_current)))
 	  { return std::__invoke(*_M_parent->_M_fun, *_M_current); }
 
 	  constexpr _Iterator&
@@ -1837,8 +1827,13 @@ namespace views
 	  { return __x._M_current - __y._M_current; }
 
 	  friend constexpr decltype(auto)
-	  iter_move(const _Iterator& __i) noexcept(noexcept(__iter_move(__i)))
-	  { return __iter_move(__i); }
+	  iter_move(const _Iterator& __i) noexcept(noexcept(*__i))
+	  {
+	    if constexpr (is_lvalue_reference_v<decltype(*__i)>)
+	      return std::move(*__i);
+	    else
+	      return *__i;
+	  }
 
 	  friend constexpr void
 	  iter_swap(const _Iterator& __x, const _Iterator& __y)
@@ -2715,7 +2710,7 @@ namespace views
 	  //  current of outer-iterator.  current is equivalent to current_ if
 	  //  V models forward_range, and parent_->current_ otherwise.
 	  constexpr auto&
-	  __current()
+	  __current() noexcept
 	  {
 	    if constexpr (forward_range<_Vp>)
 	      return _M_current;
@@ -2724,7 +2719,7 @@ namespace views
 	  }
 
 	  constexpr auto&
-	  __current() const
+	  __current() const noexcept
 	  {
 	    if constexpr (forward_range<_Vp>)
 	      return _M_current;
@@ -2860,7 +2855,7 @@ namespace views
 	    auto __end = ranges::end(_M_i._M_parent->_M_base);
 	    if constexpr (__detail::__tiny_range<_Pattern>)
 	      {
-		const auto& __cur = _M_i.__current();
+		const auto& __cur = _M_i_current();
 		if (__cur == __end)
 		  return true;
 		if (__pcur == __pend)
@@ -2869,7 +2864,7 @@ namespace views
 	      }
 	    else
 	      {
-		auto __cur = _M_i.__current();
+		auto __cur = _M_i_current();
 		if (__cur == __end)
 		  return true;
 		if (__pcur == __pend)
@@ -2896,16 +2891,13 @@ namespace views
 	      return _Cat{};
 	  }
 
-	  static constexpr decltype(auto)
-	  __iter_move(const _InnerIter& __i = {})
-	  noexcept(noexcept(ranges::iter_move(__i._M_i.__current())))
-	  { return ranges::iter_move(__i._M_i.__current()); }
+	  constexpr auto&
+	  _M_i_current() noexcept
+	  { return _M_i.__current(); }
 
-	  static constexpr void
-	  __iter_swap(const _InnerIter& __x = {}, const _InnerIter& __y = {})
-	    noexcept(noexcept(ranges::iter_swap(__x._M_i.__current(),
-						__y._M_i.__current())))
-	  { ranges::iter_swap(__x._M_i.__current(), __y._M_i.__current()); }
+	  constexpr auto&
+	  _M_i_current() const noexcept
+	  { return _M_i.__current(); }
 
 	  _OuterIter<_Const> _M_i = _OuterIter<_Const>();
 	  bool _M_incremented = false;
@@ -2926,7 +2918,7 @@ namespace views
 
 	  constexpr decltype(auto)
 	  operator*() const
-	  { return *_M_i._M_current; }
+	  { return *_M_i_current(); }
 
 	  constexpr _InnerIter&
 	  operator++()
@@ -2935,7 +2927,7 @@ namespace views
 	    if constexpr (!forward_range<_Base>)
 	      if constexpr (_Pattern::size() == 0)
 		return *this;
-	    ++_M_i.__current();
+	    ++_M_i_current();
 	    return *this;
 	  }
 
@@ -2962,14 +2954,16 @@ namespace views
 	  { return __x.__at_end(); }
 
 	  friend constexpr decltype(auto)
-	  iter_move(const _InnerIter& __i) noexcept(noexcept(__iter_move()))
-	  { return __iter_move(__i); }
+	  iter_move(const _InnerIter& __i)
+	    noexcept(noexcept(ranges::iter_move(__i._M_i_current())))
+	  { return ranges::iter_move(__i._M_i_current()); }
 
 	  friend constexpr void
 	  iter_swap(const _InnerIter& __x, const _InnerIter& __y)
-	    noexcept(noexcept(__iter_swap()))
+	    noexcept(noexcept(ranges::iter_swap(__x._M_i_current(),
+						__y._M_i_current())))
 	    requires indirectly_swappable<iterator_t<_Base>>
-	  { __iter_swap(__x, __y); }
+	  { ranges::iter_swap(__x._M_i_current(), __y._M_i_current()); }
 	};
 
       _Vp _M_base = _Vp();
@@ -3010,8 +3004,8 @@ namespace views
       begin()
       {
 	if constexpr (forward_range<_Vp>)
-	  return _OuterIter<__detail::__simple_view<_Vp>>{*this,
-							ranges::begin(_M_base)};
+	  return _OuterIter<__detail::__simple_view<_Vp>>{
+	      *this, ranges::begin(_M_base)};
 	else
 	  {
 	    _M_current = ranges::begin(_M_base);
@@ -3028,7 +3022,8 @@ namespace views
       constexpr auto
       end() requires forward_range<_Vp> && common_range<_Vp>
       {
-	return _OuterIter<__detail::__simple_view<_Vp>>{*this, ranges::end(_M_base)};
+	return _OuterIter<__detail::__simple_view<_Vp>>{
+	    *this, ranges::end(_M_base)};
       }
 
       constexpr auto
diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/split.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/split.cc
index e7556725e4f..abdbfb41d8b 100644
--- a/libstdc++-v3/testsuite/std/ranges/adaptors/split.cc
+++ b/libstdc++-v3/testsuite/std/ranges/adaptors/split.cc
@@ -121,6 +121,18 @@ test06()
   b = ranges::begin(v);
 }
 
+void
+test07()
+{
+  char str[] = "banana split";
+  auto split = str | views::split(' ');
+  auto val = *split.begin();
+  auto b = val.begin();
+  auto b2 = b++;
+  static_assert( noexcept(iter_move(b)) );
+  static_assert( noexcept(iter_swap(b, b2)) );
+}
+
 int
 main()
 {
@@ -130,4 +142,5 @@ main()
   test04();
   test05();
   test06();
+  test07();
 }
Kees Cook via Gcc-patches March 10, 2020, 10:16 p.m. | #2
On 10/03/20 17:52 +0000, Jonathan Wakely wrote:
>On 10/03/20 11:40 +0000, Jonathan Wakely wrote:

>>G++ fails to diagnose this non-dependent expression, but Clang doesn't

>>like it.

>>

>>	PR c++/94117

>>	* include/std/ranges (ranges::transform_view::_Iterator::iter_move):

>>	Change expression in noexcept-specifier to match function body.

>>

>

>

>This patch goes further and removes the __iter_move helper completely,

>and the __iter_swap one, in transform_view.

>

>It also does the same in split_view, and fixes a bug where the

>noexcept-specifier was always false.

>

>I've also added new _M_i_current() accessors (overloaded for const and

>non-const) to return _M_i.__current(). Using this instead of

>_M_i._M_current fixes a bug in inner-iterator::operator*() (which is

>also present in the working draft).


I missed a few more bugs in the outer iterator, where _M_current was
being used directly despite only being valid for forward ranges. Fixed
by this patch.

Tested powerpc64le-linux, committed to master.
commit 0b7f1e24316cfc1f85408918d1734d3266d65089
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Mar 10 22:15:58 2020 +0000

    libstdc++: Fix uses of _M_current in split_view's outer iterator
    
    These direct uses of _M_current should all be __current() so they are
    valid when the base type doesn't satisfy the forward_range concept.
    
            * include/std/ranges (split_view::_OuterIter::__at_end): Use __current
            instead of _M_current.
            (split_view::_OuterIter::operator++): Likewise.

diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
index 4dc7342e2f7..de120d6b55d 100644
--- a/libstdc++-v3/include/std/ranges
+++ b/libstdc++-v3/include/std/ranges
@@ -2703,9 +2703,9 @@ namespace views
 
 	  constexpr bool
 	  __at_end() const
-	  { return _M_current == ranges::end(_M_parent->_M_base); }
+	  { return __current() == ranges::end(_M_parent->_M_base); }
 
-	  // XXX: [24.7.11.3.1]
+	  // [range.split.outer] p1
 	  //  Many of the following specifications refer to the notional member
 	  //  current of outer-iterator.  current is equivalent to current_ if
 	  //  V models forward_range, and parent_->current_ otherwise.
@@ -2798,21 +2798,21 @@ namespace views
 	  operator++()
 	  {
 	    const auto __end = ranges::end(_M_parent->_M_base);
-	    if (_M_current == __end)
+	    if (__current() == __end)
 	      return *this;
 	    const auto [__pbegin, __pend] = subrange{_M_parent->_M_pattern};
 	    if (__pbegin == __pend)
-	      ++_M_current;
+	      ++__current();
 	    else
 	      do
 		{
 		  auto [__b, __p]
-		    = __detail::mismatch(std::move(_M_current), __end,
+		    = __detail::mismatch(std::move(__current()), __end,
 					 __pbegin, __pend);
-		  _M_current = std::move(__b);
+		  __current() = std::move(__b);
 		  if (__p == __pend)
 		    break;
-		} while (++_M_current != __end);
+		} while (++__current() != __end);
 	    return *this;
 	  }

Patch

diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
index eb54b110c04..292132db990 100644
--- a/libstdc++-v3/include/std/ranges
+++ b/libstdc++-v3/include/std/ranges
@@ -1837,7 +1837,7 @@  namespace views
 	  { return __x._M_current - __y._M_current; }
 
 	  friend constexpr decltype(auto)
-	  iter_move(const _Iterator& __i) noexcept(noexcept(__iter_move()))
+	  iter_move(const _Iterator& __i) noexcept(noexcept(__iter_move(__i)))
 	  { return __iter_move(__i); }
 
 	  friend constexpr void