[Patch] SFINAE on is_same first in variant's _Tp&& constructor

[Patch] SFINAE on is_same first in variant's _Tp&& constructor

[Tim Shen via gcc-patches]

[-- Attachment #1: Type: text/plain, Size: 229 bytes --]

This fixes PR libstdc++/80737.

I actually can't come up with a minimal test case, because I suspect
that there is a front-end bug in GCC. See discussions in the bug.

Tested on x86_64-linux-gnu.

Thanks!


-- 
Regards,
Tim Shen

[-- Attachment #2: a.diff --]
[-- Type: text/x-patch, Size: 2329 bytes --]

commit 6f362991f025069328c4901d95b657d498aad250
Author: Tim Shen <timshen@google.com>
Date:   Fri May 19 22:26:58 2017 -0700

    2017-05-20  Tim Shen  <timshen@google.com>
    
            PR libstdc++/80737
            * include/std/variant(variant::variant): SFINAE on is_same first.
            * testsuite/20_util/variant/any.cc: test case.

diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 0e04a820d69..b9824a5182c 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -936,9 +936,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       noexcept((is_nothrow_move_constructible_v<_Types> && ...)) = default;
 
       template<typename _Tp,
+	       typename = enable_if_t<!is_same_v<decay_t<_Tp>, variant>>,
 	       typename = enable_if_t<__exactly_once<__accepted_type<_Tp&&>>
-			  && is_constructible_v<__accepted_type<_Tp&&>, _Tp&&>
-			  && !is_same_v<decay_t<_Tp>, variant>>>
+			  && is_constructible_v<__accepted_type<_Tp&&>, _Tp&&>>>
 	constexpr
 	variant(_Tp&& __t)
 	noexcept(is_nothrow_constructible_v<__accepted_type<_Tp&&>, _Tp&&>)
diff --git a/libstdc++-v3/testsuite/20_util/variant/any.cc b/libstdc++-v3/testsuite/20_util/variant/any.cc
new file mode 100644
index 00000000000..5811d0f055e
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/variant/any.cc
@@ -0,0 +1,31 @@
+// { dg-options "-std=gnu++17" }
+// { dg-do compile }
+
+// Copyright (C) 2017 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 <any>
+#include <variant>
+
+struct A { std::variant<std::any> a; };
+
+void Bar(const A&);
+
+void Foo() {
+  A a;
+  Bar(a);
+}

Re: [Patch] SFINAE on is_same first in variant's _Tp&& constructor

[Jonathan Wakely]

On 19/05/17 22:40 -0700, Tim Shen via libstdc++ wrote:
>diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
>index 0e04a820d69..b9824a5182c 100644
>--- a/libstdc++-v3/include/std/variant
>+++ b/libstdc++-v3/include/std/variant
>@@ -936,9 +936,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       noexcept((is_nothrow_move_constructible_v<_Types> && ...)) = default;
> 
>       template<typename _Tp,
>+	       typename = enable_if_t<!is_same_v<decay_t<_Tp>, variant>>,
> 	       typename = enable_if_t<__exactly_once<__accepted_type<_Tp&&>>
>-			  && is_constructible_v<__accepted_type<_Tp&&>, _Tp&&>
>-			  && !is_same_v<decay_t<_Tp>, variant>>>
>+			  && is_constructible_v<__accepted_type<_Tp&&>, _Tp&&>>>

Does this definitely short-circuit? I seem to recall a similar case
where either Clang or GCC (I think it was Clang) was evaluating the
second default template argument even though the first had produce a
substition failure.

If we need to guarantee it short-circuits then we'd want:

      template<typename _Tp,
 	       typename = enable_if_t<__and_<
                                      __not_<is_same<decay_t<_Tp>, variant>>,
                                      __bool_constant<
                                        __exactly_once<__accepted_type<_Tp&&>>
                                        && is_constructible_v<__accepted_type<_Tp&&>, _Tp&&>>>

i.e. use __and_< is-this-type, everything-else> where
"everything-else" still uses && to avoid making the instantiations too
deep.

Also, this is another place where we could use an __is_samey<T, U>
trait that does is_same<remove_cv_t<remove_reference_t<T>, U>.

Re: [Patch] SFINAE on is_same first in variant's _Tp&& constructor

[Tim Shen via gcc-patches]

On Mon, May 22, 2017 at 6:21 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 19/05/17 22:40 -0700, Tim Shen via libstdc++ wrote:
>>
>> diff --git a/libstdc++-v3/include/std/variant
>> b/libstdc++-v3/include/std/variant
>> index 0e04a820d69..b9824a5182c 100644
>> --- a/libstdc++-v3/include/std/variant
>> +++ b/libstdc++-v3/include/std/variant
>> @@ -936,9 +936,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>       noexcept((is_nothrow_move_constructible_v<_Types> && ...)) =
>> default;
>>
>>       template<typename _Tp,
>> +              typename = enable_if_t<!is_same_v<decay_t<_Tp>, variant>>,
>>                typename =
>> enable_if_t<__exactly_once<__accepted_type<_Tp&&>>
>> -                         && is_constructible_v<__accepted_type<_Tp&&>,
>> _Tp&&>
>> -                         && !is_same_v<decay_t<_Tp>, variant>>>
>> +                         && is_constructible_v<__accepted_type<_Tp&&>,
>> _Tp&&>>>
>
>
> Does this definitely short-circuit? I seem to recall a similar case
> where either Clang or GCC (I think it was Clang) was evaluating the
> second default template argument even though the first had produce a
> substition failure.
>
> If we need to guarantee it short-circuits then we'd want:
>
>      template<typename _Tp,
>                typename = enable_if_t<__and_<
>                                      __not_<is_same<decay_t<_Tp>, variant>>,
>                                      __bool_constant<
>
> __exactly_once<__accepted_type<_Tp&&>>
>                                        &&
> is_constructible_v<__accepted_type<_Tp&&>, _Tp&&>>>
>
> i.e. use __and_< is-this-type, everything-else> where
> "everything-else" still uses && to avoid making the instantiations too
> deep.

Good observation. I changed to use __and_ and __not_:

-              typename = enable_if_t<__exactly_once<__accepted_type<_Tp&&>>
-                         && is_constructible_v<__accepted_type<_Tp&&>, _Tp&&>
-                         && !is_same_v<decay_t<_Tp>, variant>>>
+              typename = enable_if_t<__and_<
+                             __not_<is_same<decay_t<_Tp>, variant>>,
+                             std::integral_constant<bool,
+                                 __exactly_once<__accepted_type<_Tp&&>>>,
+                             is_constructible<__accepted_type<_Tp&&>,
_Tp&&>>::value>>

(I didn't use && at all, just to verify the correctness)

but the compile still fails with the similar error messages. If __and_
and __not_ are expected to work, then the root cause is unlikely "not
short-circuit" only.

I suggest to cc a front-end person (Jason?) to take a look, as I
suggested in the bug, and the example: https://godbolt.org/g/AxUv16.

>
> Also, this is another place where we could use an __is_samey<T, U>
> trait that does is_same<remove_cv_t<remove_reference_t<T>, U>.
>

I never know that "samey" is a word. :)


-- 
Regards,
Tim Shen

Re: [Patch] SFINAE on is_same first in variant's _Tp&& constructor

[Tim Shen via gcc-patches]

On Mon, May 22, 2017 at 11:05 AM, Tim Shen <timshen@google.com> wrote:
> On Mon, May 22, 2017 at 6:21 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
> I suggest to cc a front-end person (Jason?) to take a look, as I
> suggested in the bug, and the example: https://godbolt.org/g/AxUv16.

See more discussion in pr80737. Basically in the godbolt example,
turning on and off -DBUG results in different behaviors, but it
shouldn't.


-- 
Regards,
Tim Shen

Re: [Patch] SFINAE on is_same first in variant's _Tp&& constructor

[Tim Song]

On Mon, May 22, 2017 at 9:21 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 19/05/17 22:40 -0700, Tim Shen via libstdc++ wrote:
>>
>> diff --git a/libstdc++-v3/include/std/variant
>> b/libstdc++-v3/include/std/variant
>> index 0e04a820d69..b9824a5182c 100644
>> --- a/libstdc++-v3/include/std/variant
>> +++ b/libstdc++-v3/include/std/variant
>> @@ -936,9 +936,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>       noexcept((is_nothrow_move_constructible_v<_Types> && ...)) =
>> default;
>>
>>       template<typename _Tp,
>> +              typename = enable_if_t<!is_same_v<decay_t<_Tp>, variant>>,
>>                typename =
>> enable_if_t<__exactly_once<__accepted_type<_Tp&&>>
>> -                         && is_constructible_v<__accepted_type<_Tp&&>,
>> _Tp&&>
>> -                         && !is_same_v<decay_t<_Tp>, variant>>>
>> +                         && is_constructible_v<__accepted_type<_Tp&&>,
>> _Tp&&>>>
>
>
> Does this definitely short-circuit? I seem to recall a similar case
> where either Clang or GCC (I think it was Clang) was evaluating the
> second default template argument even though the first had produce a
> substition failure.
>
> If we need to guarantee it short-circuits then we'd want:
>
>      template<typename _Tp,
>                typename = enable_if_t<__and_<
>                                      __not_<is_same<decay_t<_Tp>, variant>>,
>                                      __bool_constant<
>                                        __exactly_once<__accepted_type<_Tp&&>>
>                                        && is_constructible_v<__accepted_type<_Tp&&>, _Tp&&>>>
>
> i.e. use __and_< is-this-type, everything-else> where
> "everything-else" still uses && to avoid making the instantiations too
> deep.
>
> Also, this is another place where we could use an __is_samey<T, U>
> trait that does is_same<remove_cv_t<remove_reference_t<T>, U>.
>

The thing that needs to be short-circuited out is the evaluation of
__accepted_type<_Tp&&>, which starts the tower of turtles.

The original patch does that (assuming core issue 1227's resolution),
but the __and_ version doesn't; __and_ only short circuits the
immediate parameter, not things used in forming it.

Re: [Patch] SFINAE on is_same first in variant's _Tp&& constructor

[Tim Song]

On Mon, May 22, 2017 at 4:14 PM, Tim Song <t.canens.cpp@gmail.com> wrote:
> assuming core issue 1227's resolution

Actually, 1227 doesn't touch default template arguments :( OTOH, the
paragraph dealing with default template arguments seems to be full of
issues - it says "invalid type" rather than "invalid type or
expression", and "above" when the description is actually "below".

Anyway, that should be easily fixable by moving the SFINAE into the
type of a non-type parameter, I think.

Re: [Patch] SFINAE on is_same first in variant's _Tp&& constructor

[Jonathan Wakely]

On 22/05/17 16:14 -0400, Tim Song wrote:
>On Mon, May 22, 2017 at 9:21 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
>> On 19/05/17 22:40 -0700, Tim Shen via libstdc++ wrote:
>>>
>>> diff --git a/libstdc++-v3/include/std/variant
>>> b/libstdc++-v3/include/std/variant
>>> index 0e04a820d69..b9824a5182c 100644
>>> --- a/libstdc++-v3/include/std/variant
>>> +++ b/libstdc++-v3/include/std/variant
>>> @@ -936,9 +936,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>>       noexcept((is_nothrow_move_constructible_v<_Types> && ...)) =
>>> default;
>>>
>>>       template<typename _Tp,
>>> +              typename = enable_if_t<!is_same_v<decay_t<_Tp>, variant>>,
>>>                typename =
>>> enable_if_t<__exactly_once<__accepted_type<_Tp&&>>
>>> -                         && is_constructible_v<__accepted_type<_Tp&&>,
>>> _Tp&&>
>>> -                         && !is_same_v<decay_t<_Tp>, variant>>>
>>> +                         && is_constructible_v<__accepted_type<_Tp&&>,
>>> _Tp&&>>>
>>
>>
>> Does this definitely short-circuit? I seem to recall a similar case
>> where either Clang or GCC (I think it was Clang) was evaluating the
>> second default template argument even though the first had produce a
>> substition failure.
>>
>> If we need to guarantee it short-circuits then we'd want:
>>
>>      template<typename _Tp,
>>                typename = enable_if_t<__and_<
>>                                      __not_<is_same<decay_t<_Tp>, variant>>,
>>                                      __bool_constant<
>>                                        __exactly_once<__accepted_type<_Tp&&>>
>>                                        && is_constructible_v<__accepted_type<_Tp&&>, _Tp&&>>>
>>
>> i.e. use __and_< is-this-type, everything-else> where
>> "everything-else" still uses && to avoid making the instantiations too
>> deep.
>>
>> Also, this is another place where we could use an __is_samey<T, U>
>> trait that does is_same<remove_cv_t<remove_reference_t<T>, U>.
>>
>
>The thing that needs to be short-circuited out is the evaluation of
>__accepted_type<_Tp&&>, which starts the tower of turtles.

Ah I see.

>The original patch does that (assuming core issue 1227's resolution),
>but the __and_ version doesn't; __and_ only short circuits the
>immediate parameter, not things used in forming it.

Then the original patch is OK for trunk and gcc-7-branch.

Thank you Tim and Tim for the explanations.

Re: [Patch] SFINAE on is_same first in variant's _Tp&& constructor

[Tim Shen via gcc-patches]

On Tue, May 23, 2017 at 3:24 AM, Jonathan Wakely wrote:
> On 22/05/17 16:14 -0400, Tim Song wrote:
> Ah I see.
>
>> The original patch does that (assuming core issue 1227's resolution),
>> but the __and_ version doesn't; __and_ only short circuits the
>> immediate parameter, not things used in forming it.
>
>
> Then the original patch is OK for trunk and gcc-7-branch.
>
> Thank you Tim and Tim for the explanations.
>

Committed. I didn't bother using remove_cv<remove_reference<T>> only
because p0088r3 says decay_t.


-- 
Regards,
Tim Shen