[PATCH] PR libstdc++/71579 assert that type traits are not misused with an incomplete type

[Jonathan Wakely]

On 30/05/19 09:06 +0300, Antony Polukhin wrote:
>чт, 9 мая 2019 г. в 00:10, Jonathan Wakely <jwakely@redhat.com>:
>>
>> On 06/05/19 14:19 +0300, Antony Polukhin wrote:
>> >@@ -924,14 +984,25 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> >   template<typename _Tp>
>> >     struct is_default_constructible
>> >     : public __is_default_constructible_safe<_Tp>::type
>> >-    { };
>> >+    {
>> >+      static_assert(std::__is_complete_or_unbounded(__type_identity<_Tp>{}),
>> >+      "template argument must be a complete class or an unbounded array");
>> >+    };
>> >
>> >   /// is_constructible
>>
>> This "///" comment is for Doxygen, and should remain with the
>> is_constructible trait, not __is_constructible_impl.
>
>Fixed that issue along with some other misplaced Doxygen comments.
>Rebased the patch and removed some trailing white-spaces.

Thanks! Rebasing this was on my TODO list for today.

>-- 
>Best regards,
>Antony Polukhin

>diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
>index b209460..c5f0672 100644
>--- a/libstdc++-v3/ChangeLog
>+++ b/libstdc++-v3/ChangeLog
>@@ -1,3 +1,27 @@
>+2019-05-29  Antony Polukhin  <antoshkka@gmail.com>
>+
>+	PR libstdc++/71579
>+	* include/std/type_traits: Add static_asserts to make sure that
>+	type traits are not misused with an incomplete type.

This changelog could be more explicit. It doesn't have to list every
changed trait, but maybe something like:

	* include/std/type_traits: Add static_asserts to make sure that
	type traits are not misused with an incomplete type. Create
        new base characteristics without assertions that can be reused
        in other traits.

>+	* testsuite/20_util/__is_complete_or_unbounded/memoization.cc:
>+	New test.
>+	* testsuite/20_util/__is_complete_or_unbounded/memoization_neg.cc:
>+	Likewise.
>+	* testsuite/20_util/__is_complete_or_unbounded/value.cc: Likewise.

I think I'd prefer not to have the double underscores in the directory
name here.

>@@ -876,19 +935,38 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>   template<typename _Tp>
>     struct is_nothrow_destructible
>     : public __is_nt_destructible_safe<_Tp>::type
>+    {
>+      static_assert(std::__is_complete_or_unbounded(__type_identity<_Tp>{}),
>+	"template argument must be a complete class or an unbounded array");
>+    };
>+
>+    template<typename _Tp, typename... _Args>

N.B. This 'template' keyword should only be indented two spaces,
however ...

>+    struct __is_constructible_impl
>+      : public __bool_constant<__is_constructible(_Tp, _Args...)>
>     { };

I thought this could be an alias template instead of a class template:

  template<typename _Tp, typename... _Args>
    using __is_constructible_impl
      = __bool_constant<__is_constructible(_Tp, _Args...)>;

But it causes failures in the 20_util/variable_templates_for_traits.cc
test. It looks like it instantiates some things too eagerly if it's an
alias template.

>   /// is_constructible
>   template<typename _Tp, typename... _Args>
>     struct is_constructible
>-      : public __bool_constant<__is_constructible(_Tp, _Args...)>
>+      : public __is_constructible_impl<_Tp, _Args...>
>+    {
>+      static_assert(std::__is_complete_or_unbounded(__type_identity<_Tp>{}),
>+	"template argument must be a complete class or an unbounded array");
>+    };
>+
>+  template<typename _Tp>
>+    struct __is_default_constructible_impl
>+    : public __is_constructible_impl<_Tp>::type
>     { };

Do we need this new __is_default_constructible_impl type?
Could we just use __is_constructible_impl<_Tp> instead, to avoid an
extra step (and extra template instantiations)?

>@@ -946,12 +1030,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     : public __is_nt_default_constructible_atom<_Tp>
>     { };
>
>+    template<typename _Tp>

Indentation again :-)

>+    struct __is_nothrow_default_constructible_impl
>+    : public __and_<__is_default_constructible_impl<_Tp>,
>+                    __is_nt_default_constructible_impl<_Tp>>
>+    { };

This one can be an alias template:

  template<typename _Tp>
    using __is_nothrow_default_constructible_impl
      = typename __and_<__is_constructible_impl<_Tp>,
			__is_nt_default_constructible_impl<_Tp>>::type;

>+    template<typename _Tp, typename _Up>
>+    struct __is_nothrow_assignable_impl
>+    : public __and_<__bool_constant<__is_assignable(_Tp, _Up)>,
>+		    __is_nt_assignable_impl<_Tp, _Up>>
>+    { };

I wanted this one to be an alias template:

  template<typename _Tp, typename _Up>
    using __is_nothrow_assignable_impl
      = __and_<__bool_constant<__is_assignable(_Tp, _Up)>,
               __is_nt_assignable_impl<_Tp, _Up>>;

But that also causes the same test to fail. I think it would work if
we moved the __is_assignable intrinsic into a new __is_assignable_impl
class template, but then we'd be adding a new class template just to
get rid of another one. That doesn't seem sensible.

I've attached a relative diff, to be applied on top of yours, with my
suggested tweaks. Do you see any issues with it?

(If you're happy with those tweaks I can go ahead and apply this,
there's no need for an updated patch from you).

-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.txt
Type: text/x-patch
Size: 5875 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20190530/905083fd/attachment.bin>