[v3] PR libstdc++/87855

Message ID CAFk2RUYYjWeznCLcoz_XHKHvfCb+5Tb2Y4BkLPiBFVLvZbWcLw@mail.gmail.com
State New
Headers show
Series
  • [v3] PR libstdc++/87855
Related show

Commit Message

Ville Voutilainen Nov. 15, 2018, noon
Tested on Linux-PPC64. Ok for trunk? Backports?

2018-11-15  Ville Voutilainen  <ville.voutilainen@gmail.com>

    PR libstdc++/87855
    Also implement P0602R4 (variant and optional
    should propagate copy/move triviality) for std::optional.
    * include/std/optional (_Optional_payload): Change
    the main constraints to check constructibility in
    addition to assignability.
    (operator=): Make constexpr.
    (_M_reset): Likewise.
    (_M_construct): Likewise.
    (operator->): Likewise.
    * testsuite/20_util/optional/assignment/8.cc: Adjust.
    * testsuite/20_util/optional/assignment/9.cc: New.

Comments

Jonathan Wakely Nov. 19, 2018, 12:28 p.m. | #1
On 15/11/18 14:00 +0200, Ville Voutilainen wrote:
>Tested on Linux-PPC64. Ok for trunk? Backports?


One problem ...

>--- /dev/null

>+++ b/libstdc++-v3/testsuite/20_util/optional/assignment/9.cc

>@@ -0,0 +1,98 @@

>+// { dg-options "-std=gnu++17" }

>+// { dg-do compile }


This needs to be:

// { dg-do compile { target c++17 } }

Otherwise it FAILs with DejaGnu 1.5.2 and older when using something
like --target_board=unix/-std=c++14 (because old versions of DejaGnu
override the explicit dg-options in the testcase with the
--target_board options).

If you use { target c++17 } then when the old DejaGnu overrides your
{ dg-options "-std=gnu++17" } setting it still correctly skips the
test for older dialects, instead of getting a FAIL.

OK for trunk with that change, thanks.

I think it's OK to backport, but maybe let's wait a few weeks. Keep
the bugzilla open for now, until you do that backport.
Jonathan Wakely Nov. 19, 2018, 1 p.m. | #2
On 19/11/18 12:28 +0000, Jonathan Wakely wrote:
>On 15/11/18 14:00 +0200, Ville Voutilainen wrote:

>>Tested on Linux-PPC64. Ok for trunk? Backports?

>

>One problem ...

>

>>--- /dev/null

>>+++ b/libstdc++-v3/testsuite/20_util/optional/assignment/9.cc

>>@@ -0,0 +1,98 @@

>>+// { dg-options "-std=gnu++17" }

>>+// { dg-do compile }

>

>This needs to be:

>

>// { dg-do compile { target c++17 } }

>

>Otherwise it FAILs with DejaGnu 1.5.2 and older when using something

>like --target_board=unix/-std=c++14 (because old versions of DejaGnu

>override the explicit dg-options in the testcase with the

>--target_board options).

>

>If you use { target c++17 } then when the old DejaGnu overrides your

>{ dg-options "-std=gnu++17" } setting it still correctly skips the

>test for older dialects, instead of getting a FAIL.


I'm wrong about that. The check_effective_target_c++* checks in
gcc/testsuite/lib/target-supports.exp say:

# This assumes that the default for the compiler is $cxx_default, and that
# there will never be multiple -std= arguments on the command line.

So if dg-options adds -std=gnu++17 then the testsuite thinks that
option is in effect, even if there's a later -std=c++14 on the final
command-line that came from --target_board. Drat. So using an old
DejaGnu means setting -std=* via --target_board will give lots of
FAILures.

But having the { target c++17 } present is still preferable, as it
makes the test's requirements explicit, and makes the test more
futureproof. If we later change the compiler default to -std=gnu++17
then we'll want to remove the { dg-options "-std=gnu++17" } from these
tests, so that they can also be tested in C++2a and C++2b modes. If we
do that, we'll need the { target c++17 } so it doesn't get tested for
c++14 and older.

Patch

diff --git a/libstdc++-v3/include/std/optional b/libstdc++-v3/include/std/optional
index d0257c0..fefd9a3 100644
--- a/libstdc++-v3/include/std/optional
+++ b/libstdc++-v3/include/std/optional
@@ -103,10 +103,12 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template <typename _Tp,
 	    bool /*_HasTrivialDestructor*/ =
 	      is_trivially_destructible_v<_Tp>,
-	    bool /*_HasTrivialCopyAssignment*/ =
-	      is_trivially_copy_assignable_v<_Tp>,
-	    bool /*_HasTrivialMoveAssignment*/ =
-	      is_trivially_move_assignable_v<_Tp>>
+	    bool /*_HasTrivialCopy */ =
+	      is_trivially_copy_assignable_v<_Tp>
+	      && is_trivially_copy_constructible_v<_Tp>,
+	    bool /*_HasTrivialMove */ =
+	      is_trivially_move_assignable_v<_Tp>
+	      && is_trivially_move_constructible_v<_Tp>>
     struct _Optional_payload
     {
       constexpr _Optional_payload() noexcept : _M_empty() { }
@@ -148,6 +150,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  this->_M_construct(std::move(__other._M_payload));
       }
 
+      constexpr
       _Optional_payload&
       operator=(const _Optional_payload& __other)
       {
@@ -163,6 +166,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	return *this;
       }
 
+      constexpr
       _Optional_payload&
       operator=(_Optional_payload&& __other)
       noexcept(__and_v<is_nothrow_move_constructible<_Tp>,
@@ -216,6 +220,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       { return this->_M_payload; }
 
       // _M_reset is a 'safe' operation with no precondition.
+      constexpr
       void
       _M_reset() noexcept
       {
@@ -346,6 +351,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _Optional_payload(const _Optional_payload&) = default;
       _Optional_payload(_Optional_payload&&) = default;
 
+      constexpr
       _Optional_payload&
       operator=(const _Optional_payload& __other)
       {
@@ -394,6 +400,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       { return this->_M_payload; }
 
       // _M_reset is a 'safe' operation with no precondition.
+      constexpr
       void
       _M_reset() noexcept
       {
@@ -466,6 +473,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _Optional_payload&
       operator=(const _Optional_payload& __other) = default;
 
+      constexpr
       _Optional_payload&
       operator=(_Optional_payload&& __other)
       noexcept(__and_v<is_nothrow_move_constructible<_Tp>,
@@ -513,6 +521,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       { return this->_M_payload; }
 
       // _M_reset is a 'safe' operation with no precondition.
+      constexpr
       void
       _M_reset() noexcept
       {
@@ -581,6 +590,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _Optional_payload(const _Optional_payload&) = default;
       _Optional_payload(_Optional_payload&&) = default;
 
+      constexpr
       _Optional_payload&
       operator=(const _Optional_payload& __other)
       {
@@ -596,6 +606,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	return *this;
       }
 
+      constexpr
       _Optional_payload&
       operator=(_Optional_payload&& __other)
       noexcept(__and_v<is_nothrow_move_constructible<_Tp>,
@@ -624,6 +635,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       bool _M_engaged;
 
       template<typename... _Args>
+        constexpr
         void
         _M_construct(_Args&&... __args)
         noexcept(is_nothrow_constructible_v<_Stored_type, _Args...>)
@@ -643,6 +655,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       { return this->_M_payload; }
 
       // _M_reset is a 'safe' operation with no precondition.
+      constexpr
       void
       _M_reset() noexcept
       {
@@ -681,6 +694,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       }
       
       // _M_reset is a 'safe' operation with no precondition.
+      constexpr
       void
       _M_reset() noexcept
       {
@@ -1217,6 +1231,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       operator->() const
       { return std::__addressof(this->_M_get()); }
 
+      constexpr
       _Tp*
       operator->()
       { return std::__addressof(this->_M_get()); }
diff --git a/libstdc++-v3/testsuite/20_util/optional/assignment/8.cc b/libstdc++-v3/testsuite/20_util/optional/assignment/8.cc
index d573137..7241b92 100644
--- a/libstdc++-v3/testsuite/20_util/optional/assignment/8.cc
+++ b/libstdc++-v3/testsuite/20_util/optional/assignment/8.cc
@@ -91,6 +91,24 @@  struct S2 {
 };
 static_assert(std::is_trivially_move_assignable_v<S2>);
 
+struct S3 {
+  S3(const S3&);
+  S3& operator=(const S3&) = default;
+};
+static_assert(std::is_trivially_copy_assignable_v<S3>);
+static_assert(std::is_copy_assignable_v<S3>);
+static_assert(!std::is_trivially_copy_assignable_v<std::optional<S3>>);
+static_assert(std::is_copy_assignable_v<std::optional<S3>>);
+
+struct S4 {
+  S4(S4&&);
+  S4& operator=(S4&&) = default;
+};
+static_assert(std::is_trivially_move_assignable_v<S4>);
+static_assert(std::is_move_assignable_v<S4>);
+static_assert(!std::is_trivially_move_assignable_v<std::optional<S4>>);
+static_assert(std::is_move_assignable_v<std::optional<S4>>);
+
 union U2 {
   char dummy;
   S2 s;
diff --git a/libstdc++-v3/testsuite/20_util/optional/assignment/9.cc b/libstdc++-v3/testsuite/20_util/optional/assignment/9.cc
new file mode 100644
index 0000000..b4f4f96
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/optional/assignment/9.cc
@@ -0,0 +1,98 @@ 
+// { dg-options "-std=gnu++17" }
+// { dg-do compile }
+
+// Copyright (C) 2018 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/>.
+
+#include <optional>
+
+constexpr bool f()
+{
+  std::optional<int> o1{42};
+  std::optional<int> o2;
+  o2 = o1;
+  return (o1 == o2);
+}
+
+constexpr bool f2()
+{
+  std::optional<int> o1{42};
+  std::optional<int> o2;
+  std::optional<int> o3;
+  o2 = o1;
+  o3 = std::move(o2);
+  return (o1 == o3);
+}
+
+void g()
+{
+  constexpr bool b = f();
+  static_assert(b);
+  constexpr bool b2 = f2();
+  static_assert(b2);
+}
+
+struct NonTrivialButConstexpr
+{
+  int dummy;
+  NonTrivialButConstexpr() = default;
+  constexpr NonTrivialButConstexpr(int val) : dummy(val) {}
+  NonTrivialButConstexpr(const NonTrivialButConstexpr&) = default;
+  NonTrivialButConstexpr(NonTrivialButConstexpr&&) = default;
+  constexpr NonTrivialButConstexpr&
+  operator=(const NonTrivialButConstexpr& other)
+  {
+    dummy = other.dummy;
+    return *this;
+  }
+  constexpr NonTrivialButConstexpr&
+  operator=(NonTrivialButConstexpr&& other)
+  {
+    dummy = other.dummy;
+    return *this;
+  }
+};
+
+constexpr bool f3()
+{
+  std::optional<NonTrivialButConstexpr> d1, d2;
+  d1 = d2;
+  std::optional<NonTrivialButConstexpr> o1{42};
+  std::optional<NonTrivialButConstexpr> o2{22};
+  o2 = o1;
+  return ((*o1).dummy == (*o2).dummy && o1->dummy == o2->dummy);
+}
+
+constexpr bool f4()
+{
+  std::optional<NonTrivialButConstexpr> d1, d2;
+  d1 = std::move(d2);
+  std::optional<NonTrivialButConstexpr> o1{42};
+  std::optional<NonTrivialButConstexpr> o2{22};
+  std::optional<NonTrivialButConstexpr> o3{33};
+  o2 = o1;
+  o3 = std::move(o2);
+  return ((*o1).dummy == (*o3).dummy && o1->dummy == o3->dummy);
+}
+
+void g2()
+{
+  constexpr bool b = f3();
+  static_assert(b);
+  constexpr bool b2 = f4();
+  static_assert(b2);
+}