[v3] Implement the missing bits of LWG 2769

Message ID CAFk2RUYrttM2F1X0sTuWyNZWDpYx2Eqam5ny8qzqYp14o5uHmg@mail.gmail.com
State New
Headers show
Series
  • [v3] Implement the missing bits of LWG 2769
Related show

Commit Message

Ville Voutilainen Feb. 25, 2018, 9:22 p.m.
Tested partially on Linux-x64, will test with the full suite on Linux-PPC64.
Ok for trunk and the gcc-7 branch? This is theoretically a breaking change
for the branch, but the committee has decided that they don't want
the support for copyable-but-not-movable types.

2018-02-25  Ville Voutilainen  <ville.voutilainen@gmail.com>

    Implement the missing bits of LWG 2769
    * include/std/any (any_cast(const any&)): Add static_assert.
    (any_cast(any&)): Likewise.
    (any_cast(any&&)): Likewise, and remove the handling
    for copyable-but-not-movable type.
    * testsuite/20_util/any/misc/any_cast.cc: Adjust.
    * testsuite/20_util/any/misc/any_cast_neg.cc: Likewise, and
    add new tests.

Comments

Jonathan Wakely Feb. 26, 2018, 8:52 p.m. | #1
On 25/02/18 23:22 +0200, Ville Voutilainen wrote:
>Tested partially on Linux-x64, will test with the full suite on Linux-PPC64.

>Ok for trunk and the gcc-7 branch? This is theoretically a breaking change

>for the branch, but the committee has decided that they don't want

>the support for copyable-but-not-movable types.

>

>2018-02-25  Ville Voutilainen  <ville.voutilainen@gmail.com>

>

>    Implement the missing bits of LWG 2769

>    * include/std/any (any_cast(const any&)): Add static_assert.

>    (any_cast(any&)): Likewise.

>    (any_cast(any&&)): Likewise, and remove the handling

>    for copyable-but-not-movable type.

>    * testsuite/20_util/any/misc/any_cast.cc: Adjust.

>    * testsuite/20_util/any/misc/any_cast_neg.cc: Likewise, and

>    add new tests.


>diff --git a/libstdc++-v3/include/std/any b/libstdc++-v3/include/std/any

>index 466b7ca..e0aba3c 100644

>--- a/libstdc++-v3/include/std/any

>+++ b/libstdc++-v3/include/std/any

>@@ -455,6 +455,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION

>     {

>       static_assert(any::__is_valid_cast<_ValueType>(),

> 	  "Template argument must be a reference or CopyConstructible type");

>+      static_assert(is_constructible_v<_ValueType,

>+		    const _AnyCast<_ValueType>&>,


This template argument should be aligned with "_ValueType" on the
previous line, not with "is_constructible".

Looking at that file, I'm also wondering if we want the alias _AnyCast
to be defined at namespace scope. It's only used in a few function
bodies, and its name is a bit misleading.

Could you just do:

  using _Up = remove_cv_t<remove_reference_t<_ValueType>>;

in the four functions that use it?

Then I think the is_constructible specializations would fit on one line
anyway.

But those are just stylistic issues, the technical side of the patch
is fine. I had to look up why we had two overloads for any_cast(any&&)
and that seems to be a leftover from experimental::any, so thanks for
cleaning that up too.
Ville Voutilainen Feb. 26, 2018, 10:50 p.m. | #2
On 26 February 2018 at 22:52, Jonathan Wakely <jwakely@redhat.com> wrote:
> But those are just stylistic issues, the technical side of the patch

> is fine. I had to look up why we had two overloads for any_cast(any&&)

> and that seems to be a leftover from experimental::any, so thanks for

> cleaning that up too.



It was added by me when we (well, I) decided to support
copyable-but-not-movable types.
See, the current specification doesn't allow getting those types out
of an rvalue any by value.
Such an operation will perform an ill-formed move, with the
two-overload solution it would've
fallen back on copying.
Weird as such types are, I thought it not-too-much-trouble to
SFINAE-hack it to work. The specification
has since been made stricter so that it doesn't leave any
implementation leeway to allow that,
so hence the removal of that lenience.
Ville Voutilainen Feb. 27, 2018, 8:13 a.m. | #3
On 26 February 2018 at 22:52, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 25/02/18 23:22 +0200, Ville Voutilainen wrote:

>>

>> Tested partially on Linux-x64, will test with the full suite on

>> Linux-PPC64.

>> Ok for trunk and the gcc-7 branch? This is theoretically a breaking change

> This template argument should be aligned with "_ValueType" on the

> previous line, not with "is_constructible".

>

> Looking at that file, I'm also wondering if we want the alias _AnyCast

> to be defined at namespace scope. It's only used in a few function

> bodies, and its name is a bit misleading.

>

> Could you just do:

>

>  using _Up = remove_cv_t<remove_reference_t<_ValueType>>;

>

> in the four functions that use it?

>

> Then I think the is_constructible specializations would fit on one line

> anyway.



Done, new patch attached.
diff --git a/libstdc++-v3/include/std/any b/libstdc++-v3/include/std/any
index 466b7ca..a37eb38 100644
--- a/libstdc++-v3/include/std/any
+++ b/libstdc++-v3/include/std/any
@@ -438,8 +438,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       return any(in_place_type<_Tp>, __il, std::forward<_Args>(__args)...);
     }
 
-  template <typename _Tp>
-    using _AnyCast = remove_cv_t<remove_reference_t<_Tp>>;
   /**
    * @brief Access the contained object.
    *
@@ -453,9 +451,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename _ValueType>
     inline _ValueType any_cast(const any& __any)
     {
+      using _Up = remove_cv_t<remove_reference_t<_ValueType>>;
       static_assert(any::__is_valid_cast<_ValueType>(),
 	  "Template argument must be a reference or CopyConstructible type");
-      auto __p = any_cast<_AnyCast<_ValueType>>(&__any);
+      static_assert(is_constructible_v<_ValueType, const _Up&>,
+	  "Template argument must be constructible from a const value.");
+      auto __p = any_cast<_Up>(&__any);
       if (__p)
 	return static_cast<_ValueType>(*__p);
       __throw_bad_any_cast();
@@ -476,37 +477,26 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename _ValueType>
     inline _ValueType any_cast(any& __any)
     {
+      using _Up = remove_cv_t<remove_reference_t<_ValueType>>;
       static_assert(any::__is_valid_cast<_ValueType>(),
 	  "Template argument must be a reference or CopyConstructible type");
-      auto __p = any_cast<_AnyCast<_ValueType>>(&__any);
+      static_assert(is_constructible_v<_ValueType, _Up&>,
+	  "Template argument must be constructible from an lvalue.");
+      auto __p = any_cast<_Up>(&__any);
       if (__p)
 	return static_cast<_ValueType>(*__p);
       __throw_bad_any_cast();
     }
 
-  template<typename _ValueType,
-           typename enable_if<!is_move_constructible<_ValueType>::value
-                              || is_lvalue_reference<_ValueType>::value,
-                              bool>::type = true>
-    inline _ValueType any_cast(any&& __any)
-    {
-      static_assert(any::__is_valid_cast<_ValueType>(),
-	  "Template argument must be a reference or CopyConstructible type");
-      auto __p = any_cast<_AnyCast<_ValueType>>(&__any);
-      if (__p)
-	return static_cast<_ValueType>(*__p);
-      __throw_bad_any_cast();
-    }
-
-  template<typename _ValueType,
-           typename enable_if<is_move_constructible<_ValueType>::value
-                              && !is_lvalue_reference<_ValueType>::value,
-                              bool>::type = false>
+  template<typename _ValueType>
     inline _ValueType any_cast(any&& __any)
     {
+      using _Up = remove_cv_t<remove_reference_t<_ValueType>>;
       static_assert(any::__is_valid_cast<_ValueType>(),
 	  "Template argument must be a reference or CopyConstructible type");
-      auto __p = any_cast<_AnyCast<_ValueType>>(&__any);
+      static_assert(is_constructible_v<_ValueType, _Up>,
+	  "Template argument must be constructible from an rvalue.");
+      auto __p = any_cast<_Up>(&__any);
       if (__p)
 	return static_cast<_ValueType>(std::move(*__p));
       __throw_bad_any_cast();
diff --git a/libstdc++-v3/testsuite/20_util/any/misc/any_cast.cc b/libstdc++-v3/testsuite/20_util/any/misc/any_cast.cc
index 45d8b63..37a24d7 100644
--- a/libstdc++-v3/testsuite/20_util/any/misc/any_cast.cc
+++ b/libstdc++-v3/testsuite/20_util/any/misc/any_cast.cc
@@ -95,15 +95,6 @@ void test03()
   VERIFY(move_count == 1);
   MoveEnabled&& m3 = any_cast<MoveEnabled&&>(any(m));
   VERIFY(move_count == 1);
-  struct MoveDeleted
-  {
-    MoveDeleted(MoveDeleted&&) = delete;
-    MoveDeleted() = default;
-    MoveDeleted(const MoveDeleted&) = default;
-  };
-  MoveDeleted md;
-  MoveDeleted&& md2 = any_cast<MoveDeleted>(any(std::move(md)));
-  MoveDeleted&& md3 = any_cast<MoveDeleted&&>(any(std::move(md)));
 }
 
 void test04()
diff --git a/libstdc++-v3/testsuite/20_util/any/misc/any_cast_neg.cc b/libstdc++-v3/testsuite/20_util/any/misc/any_cast_neg.cc
index 50a9a67..62d7aaa 100644
--- a/libstdc++-v3/testsuite/20_util/any/misc/any_cast_neg.cc
+++ b/libstdc++-v3/testsuite/20_util/any/misc/any_cast_neg.cc
@@ -20,11 +20,26 @@
 
 #include <any>
 
+using std::any;
+using std::any_cast;
+
 void test01()
 {
-  using std::any;
-  using std::any_cast;
-
   const any y(1);
-  any_cast<int&>(y); // { dg-error "invalid static_cast" "" { target { *-*-* } } 460 }
+  any_cast<int&>(y); // { dg-error "invalid static_cast" "" { target { *-*-* } } 461 }
+  // { dg-error "Template argument must be constructible from a const value" "" { target { *-*-* } } 457 }
+}
+
+void test02()
+{
+  any y(1);
+  any_cast<int&&>(y);
+  // { dg-error "Template argument must be constructible from an lvalue" "" { target { *-*-* } } 483 }
+}
+
+void test03()
+{
+  any y(1);
+  any_cast<int&>(std::move(y));  // { dg-error "invalid static_cast" "" { target { *-*-* } } 501 }
+  // { dg-error "Template argument must be constructible from an rvalue" "" { target { *-*-* } } 497 }
 }
Jonathan Wakely Feb. 27, 2018, 11:15 a.m. | #4
On 27/02/18 10:13 +0200, Ville Voutilainen wrote:
>On 26 February 2018 at 22:52, Jonathan Wakely <jwakely@redhat.com> wrote:

>> On 25/02/18 23:22 +0200, Ville Voutilainen wrote:

>>>

>>> Tested partially on Linux-x64, will test with the full suite on

>>> Linux-PPC64.

>>> Ok for trunk and the gcc-7 branch? This is theoretically a breaking change

>> This template argument should be aligned with "_ValueType" on the

>> previous line, not with "is_constructible".

>>

>> Looking at that file, I'm also wondering if we want the alias _AnyCast

>> to be defined at namespace scope. It's only used in a few function

>> bodies, and its name is a bit misleading.

>>

>> Could you just do:

>>

>>  using _Up = remove_cv_t<remove_reference_t<_ValueType>>;

>>

>> in the four functions that use it?

>>

>> Then I think the is_constructible specializations would fit on one line

>> anyway.

>

>

>Done, new patch attached.


OK for trunk and gcc-7-branch, thanks.

Patch

diff --git a/libstdc++-v3/include/std/any b/libstdc++-v3/include/std/any
index 466b7ca..e0aba3c 100644
--- a/libstdc++-v3/include/std/any
+++ b/libstdc++-v3/include/std/any
@@ -455,6 +455,9 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     {
       static_assert(any::__is_valid_cast<_ValueType>(),
 	  "Template argument must be a reference or CopyConstructible type");
+      static_assert(is_constructible_v<_ValueType,
+		    const _AnyCast<_ValueType>&>,
+	  "Template argument must be constructible from a const value.");
       auto __p = any_cast<_AnyCast<_ValueType>>(&__any);
       if (__p)
 	return static_cast<_ValueType>(*__p);
@@ -478,34 +481,23 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     {
       static_assert(any::__is_valid_cast<_ValueType>(),
 	  "Template argument must be a reference or CopyConstructible type");
+      static_assert(is_constructible_v<_ValueType,
+		    _AnyCast<_ValueType>&>,
+	  "Template argument must be constructible from an lvalue.");
       auto __p = any_cast<_AnyCast<_ValueType>>(&__any);
       if (__p)
 	return static_cast<_ValueType>(*__p);
       __throw_bad_any_cast();
     }
 
-  template<typename _ValueType,
-           typename enable_if<!is_move_constructible<_ValueType>::value
-                              || is_lvalue_reference<_ValueType>::value,
-                              bool>::type = true>
-    inline _ValueType any_cast(any&& __any)
-    {
-      static_assert(any::__is_valid_cast<_ValueType>(),
-	  "Template argument must be a reference or CopyConstructible type");
-      auto __p = any_cast<_AnyCast<_ValueType>>(&__any);
-      if (__p)
-	return static_cast<_ValueType>(*__p);
-      __throw_bad_any_cast();
-    }
-
-  template<typename _ValueType,
-           typename enable_if<is_move_constructible<_ValueType>::value
-                              && !is_lvalue_reference<_ValueType>::value,
-                              bool>::type = false>
+  template<typename _ValueType>
     inline _ValueType any_cast(any&& __any)
     {
       static_assert(any::__is_valid_cast<_ValueType>(),
 	  "Template argument must be a reference or CopyConstructible type");
+      static_assert(is_constructible_v<_ValueType,
+		    _AnyCast<_ValueType>>,
+	  "Template argument must be constructible from an rvalue.");
       auto __p = any_cast<_AnyCast<_ValueType>>(&__any);
       if (__p)
 	return static_cast<_ValueType>(std::move(*__p));
diff --git a/libstdc++-v3/testsuite/20_util/any/misc/any_cast.cc b/libstdc++-v3/testsuite/20_util/any/misc/any_cast.cc
index 45d8b63..37a24d7 100644
--- a/libstdc++-v3/testsuite/20_util/any/misc/any_cast.cc
+++ b/libstdc++-v3/testsuite/20_util/any/misc/any_cast.cc
@@ -95,15 +95,6 @@  void test03()
   VERIFY(move_count == 1);
   MoveEnabled&& m3 = any_cast<MoveEnabled&&>(any(m));
   VERIFY(move_count == 1);
-  struct MoveDeleted
-  {
-    MoveDeleted(MoveDeleted&&) = delete;
-    MoveDeleted() = default;
-    MoveDeleted(const MoveDeleted&) = default;
-  };
-  MoveDeleted md;
-  MoveDeleted&& md2 = any_cast<MoveDeleted>(any(std::move(md)));
-  MoveDeleted&& md3 = any_cast<MoveDeleted&&>(any(std::move(md)));
 }
 
 void test04()
diff --git a/libstdc++-v3/testsuite/20_util/any/misc/any_cast_neg.cc b/libstdc++-v3/testsuite/20_util/any/misc/any_cast_neg.cc
index 50a9a67..08da07d 100644
--- a/libstdc++-v3/testsuite/20_util/any/misc/any_cast_neg.cc
+++ b/libstdc++-v3/testsuite/20_util/any/misc/any_cast_neg.cc
@@ -20,11 +20,26 @@ 
 
 #include <any>
 
+using std::any;
+using std::any_cast;
+
 void test01()
 {
-  using std::any;
-  using std::any_cast;
-
   const any y(1);
-  any_cast<int&>(y); // { dg-error "invalid static_cast" "" { target { *-*-* } } 460 }
+  any_cast<int&>(y); // { dg-error "invalid static_cast" "" { target { *-*-* } } 463 }
+  // { dg-error "Template argument must be constructible from a const value" "" { target { *-*-* } } 458 }
+}
+
+void test02()
+{
+  any y(1);
+  any_cast<int&&>(y);
+  // { dg-error "Template argument must be constructible from an lvalue" "" { target { *-*-* } } 484 }
+}
+
+void test03()
+{
+  any y(1);
+  any_cast<int&>(std::move(y));  // { dg-error "invalid static_cast" "" { target { *-*-* } } 503 }
+  // { dg-error "Template argument must be constructible from an rvalue" "" { target { *-*-* } } 498 }
 }