diff mbox series

PR libstdc++/87431 fix regression introduced by r264574

Message ID 20190106205234.GA17072@redhat.com
State New
Headers show
Series PR libstdc++/87431 fix regression introduced by r264574 | expand

Commit Message

Jonathan Wakely Jan. 6, 2019, 8:52 p.m. UTC
The previous patch for PR 87431 assumed that initialing a scalar type
could not throw, but it can obtain its value via a conversion operator,
which could throw. This meant the variant could get into a valueless
state, but the valueless_by_exception() member function would always
return false.

This patch fixes it by changing the emplace members to have strong
exception safety when initializing a contained value of trivially
copyable type. The _M_valid() member gets a corresponding change to
always return true for trivially copyable types, not just scalar types.

Strong exception safety (i.e. never becoming valueless) is achieved by
only replacing the current contained value once any potentially throwing
operations have completed. If constructing the new contained value can
throw then a new std::variant object is constructed to hold it, and then
move-assigned to *this (which won't throw).

	PR libstdc++/87431
	* include/std/variant (_Variant_storage<true, _Types...>::_M_valid):
	Check is_trivially_copyable instead of is_scalar.
	(variant::emplace<N, Args>(Args&&...)): If construction of the new
	contained value can throw and its type is trivially copyable then
	construct into a temporary variant and move from it, to provide the
	strong exception safety guarantee.
	(variant::emplace<N, U, Args>(initializer_list<U>, Args&&...)):
	Likewise.
	* testsuite/20_util/variant/87431.cc: New test.
	* testsuite/20_util/variant/run.cc: Adjust test so that throwing
	conversion causes valueless state.

Tested powerpc64-linux, committed to trunk.
commit 54cf49b46d1517752290f9e2c39604f7d40784c6
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Jan 4 18:07:50 2019 +0000

    PR libstdc++/87431 fix regression introduced by r264574
    
    The previous patch for PR 87431 assumed that initialing a scalar type
    could not throw, but it can obtain its value via a conversion operator,
    which could throw. This meant the variant could get into a valueless
    state, but the valueless_by_exception() member function would always
    return false.
    
    This patch fixes it by changing the emplace members to have strong
    exception safety when initializing a contained value of trivially
    copyable type. The _M_valid() member gets a corresponding change to
    always return true for trivially copyable types, not just scalar types.
    
    Strong exception safety (i.e. never becoming valueless) is achieved by
    only replacing the current contained value once any potentially throwing
    operations have completed. If constructing the new contained value can
    throw then a new std::variant object is constructed to hold it, and then
    move-assigned to *this (which won't throw).
    
            PR libstdc++/87431
            * include/std/variant (_Variant_storage<true, _Types...>::_M_valid):
            Check is_trivially_copyable instead of is_scalar.
            (variant::emplace<N, Args>(Args&&...)): If construction of the new
            contained value can throw and its type is trivially copyable then
            construct into a temporary variant and move from it, to provide the
            strong exception safety guarantee.
            (variant::emplace<N, U, Args>(initializer_list<U>, Args&&...)):
            Likewise.
            * testsuite/20_util/variant/87431.cc: New test.
            * testsuite/20_util/variant/run.cc: Adjust test so that throwing
            conversion causes valueless state.
diff mbox series

Patch

diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 6f2205c3507..83cf99e9ae0 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -439,7 +439,7 @@  namespace __variant
       constexpr bool
       _M_valid() const noexcept
       {
-	if constexpr ((is_scalar_v<_Types> && ...))
+	if constexpr ((is_trivially_copyable_v<_Types> && ...))
 	  return true;
 	return this->_M_index != __index_type(variant_npos);
       }
@@ -1185,6 +1185,23 @@  namespace __variant
 	{
 	  static_assert(_Np < sizeof...(_Types),
 			"The index should be in [0, number of alternatives)");
+
+	  using type = variant_alternative_t<_Np, variant>;
+	  // If constructing the value can throw but move assigning it can't,
+	  // construct in a temporary and then move assign from it. This gives
+	  // the strong exception safety guarantee, ensuring we never become
+	  // valueless.
+	  if constexpr (is_trivially_copyable_v<type>
+	      && !is_nothrow_constructible_v<type, _Args...>)
+	    {
+	      // If move assignment cannot throw then we can provide the
+	      // strong exception safety guarantee, and never become valueless.
+	      variant __tmp(in_place_index<_Np>,
+			    std::forward<_Args>(__args)...);
+	      *this = std::move(__tmp);
+	      return std::get<_Np>(*this);
+	    }
+
 	  this->~variant();
 	  __try
 	    {
@@ -1208,6 +1225,20 @@  namespace __variant
 	{
 	  static_assert(_Np < sizeof...(_Types),
 			"The index should be in [0, number of alternatives)");
+
+	  using type = variant_alternative_t<_Np, variant>;
+	  if constexpr (is_trivially_copyable_v<type>
+	      && !is_nothrow_constructible_v<type, initializer_list<_Up>,
+					     _Args...>)
+	    {
+	      // If move assignment cannot throw then we can provide the
+	      // strong exception safety guarantee, and never become valueless.
+	      variant __tmp(in_place_index<_Np>, __il,
+			    std::forward<_Args>(__args)...);
+	      *this = std::move(__tmp);
+	      return std::get<_Np>(*this);
+	    }
+
 	  this->~variant();
 	  __try
 	    {
diff --git a/libstdc++-v3/testsuite/20_util/variant/87431.cc b/libstdc++-v3/testsuite/20_util/variant/87431.cc
new file mode 100644
index 00000000000..8c5c4a78154
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/variant/87431.cc
@@ -0,0 +1,71 @@ 
+// Copyright (C) 2019 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++17" }
+// { dg-do run { target c++17 } }
+
+#include <variant>
+#include <testsuite_hooks.h>
+
+void
+test01()
+{
+  struct ThrowingConversion {
+    operator int() { throw 0; }
+  };
+
+  std::variant<float, char, int> v{'a'};
+  try
+  {
+    ThrowingConversion x;
+    v.emplace<2>(x);
+    VERIFY(false);
+  }
+  catch (int)
+  {
+    VERIFY( !v.valueless_by_exception() );
+    VERIFY( v.index() == 1 );
+    VERIFY( std::get<1>(v) == 'a' );
+  }
+}
+
+void
+test02()
+{
+  struct ThrowingConstructor {
+    ThrowingConstructor(std::initializer_list<int>, char) { throw 1; }
+  };
+
+  std::variant<float, char, ThrowingConstructor> v{'a'};
+  try
+  {
+    v.emplace<2>({1, 2, 3}, '4');
+    VERIFY(false);
+  }
+  catch (int)
+  {
+    VERIFY( !v.valueless_by_exception() );
+    VERIFY( v.index() == 1 );
+    VERIFY( std::get<1>(v) == 'a' );
+  }
+}
+
+int
+main()
+{
+  test01();
+}
diff --git a/libstdc++-v3/testsuite/20_util/variant/run.cc b/libstdc++-v3/testsuite/20_util/variant/run.cc
index 124759240f9..c5ea7df37ec 100644
--- a/libstdc++-v3/testsuite/20_util/variant/run.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/run.cc
@@ -354,7 +354,7 @@  void test_hash()
   {
     struct A
     {
-      operator int()
+      operator string()
       {
         throw nullptr;
       }
@@ -362,7 +362,7 @@  void test_hash()
     variant<int, string> v;
     try
       {
-        v.emplace<0>(A{});
+        v.emplace<1>(A{});
       }
     catch (nullptr_t)
       {