Re: [Patch] SFINAE on is_same first in variant's _Tp&& constructor
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> only because p0088r3 says decay_t. -- Regards, Tim Shen
Re: [Patch] SFINAE on is_same first in variant's _Tp&& constructor
On 22/05/17 16:14 -0400, Tim Song wrote: On Mon, May 22, 2017 at 9:21 AM, Jonathan Wakely 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, variant>>, typename = enable_if_t<__exactly_once<__accepted_type<_Tp&&>> - && is_constructible_v<__accepted_type<_Tp&&>, _Tp&&> - && !is_same_v, 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, 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 trait that does is_same, 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
On Mon, May 22, 2017 at 4:14 PM, Tim Song 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
On Mon, May 22, 2017 at 9:21 AM, Jonathan Wakely 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 = enable_if_t, variant>>, >>typename = >> enable_if_t<__exactly_once<__accepted_type<_Tp&&>> >> - && is_constructible_v<__accepted_type<_Tp&&>, >> _Tp&&> >> - && !is_same_v, 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: > > templatetypename = enable_if_t<__and_< > __not_, 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 > trait that does is_same, 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
On Mon, May 22, 2017 at 11:05 AM, Tim Shen wrote: > On Mon, May 22, 2017 at 6:21 AM, Jonathan Wakely 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
On Mon, May 22, 2017 at 6:21 AM, Jonathan Wakely 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 = enable_if_t, variant>>, >>typename = >> enable_if_t<__exactly_once<__accepted_type<_Tp&&>> >> - && is_constructible_v<__accepted_type<_Tp&&>, >> _Tp&&> >> - && !is_same_v, 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: > > templatetypename = enable_if_t<__and_< > __not_, 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, variant>>> + typename = enable_if_t<__and_< + __not_, variant>>, + std::integral_constant>>, + 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 > trait that does is_same, U>. > I never know that "samey" is a word. :) -- Regards, Tim Shen
Re: [Patch] SFINAE on is_same first in variant's _Tp&& constructor
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, variant>>, typename = enable_if_t<__exactly_once<__accepted_type<_Tp&&>> - && is_constructible_v<__accepted_type<_Tp&&>, _Tp&&> - && !is_same_v, 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, 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 trait that does is_same, U>.
[Patch] SFINAE on is_same first in variant's _Tp&& constructor
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
commit 6f362991f025069328c4901d95b657d498aad250
Author: Tim Shen
Date: Fri May 19 22:26:58 2017 -0700
2017-05-20 Tim Shen
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, variant>>,
typename = enable_if_t<__exactly_once<__accepted_type<_Tp&&>>
- && is_constructible_v<__accepted_type<_Tp&&>, _Tp&&>
- && !is_same_v, 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 000..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
+//
