Message ID | CAG4ZjNkZui6RkKB-cydS+q1=xUcuNS42Uha6w-N7fK7GbEOmSw@mail.gmail.com |
---|---|
State | New |
Headers | show |
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>.
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. :)
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.
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.
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.
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.
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.
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); +}