diff mbox

SFINAE on is_same first in variant's _Tp&& constructor

Message ID CAG4ZjNkZui6RkKB-cydS+q1=xUcuNS42Uha6w-N7fK7GbEOmSw@mail.gmail.com
State New
Headers show

Commit Message

Li, Pan2 via Gcc-patches May 20, 2017, 5:40 a.m. UTC 
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!

Comments

Jonathan Wakely May 22, 2017, 1:21 p.m. UTC | #1 
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>.
Li, Pan2 via Gcc-patches May 22, 2017, 6:05 p.m. UTC | #2 
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. :)
Li, Pan2 via Gcc-patches May 22, 2017, 6:10 p.m. UTC | #3 
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.
Tim Song May 22, 2017, 8:14 p.m. UTC | #4 
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.
Tim Song May 22, 2017, 8:26 p.m. UTC | #5 
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.
Jonathan Wakely May 23, 2017, 10:24 a.m. UTC | #6 
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.
Li, Pan2 via Gcc-patches May 28, 2017, 9:29 p.m. UTC | #7 
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.
diff mbox

Patch

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);
+}