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

[Jonathan Wakely <jwakely at redhat dot com>]

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.