On Mon, 20 Apr 2020, Jason Merrill wrote:
> On 4/19/20 12:55 PM, Patrick Palka wrote:
> > This PR seems to be similar to PR c++/43382, except that the recursive call
> > to
> > the variadic function with trailing return type in this testcase is
> > additionally
> > given some explicit template arguments.
> >
> > In the first testcase below, when resolving the recursive call to 'select',
> > fn_type_unification first substitutes in the call's explicit template
> > arguments
> > before doing unification, and so during this substitution the template
> > argument
> > pack for Args is incomplete.
> >
> > Since the pack is incomplete, the substitution of 'args...' in the trailing
> > return type decltype(f(args...)) is handled by the unsubstituted_packs case
> > of
> > tsubst_pack_expansion. But the handling of this case happens _before_ we
> > reset
> > local_specializations, and so the substitution ends up reusing the old
> > binding
> > for 'args' from local_specializations rather than building a new one.
> >
> > This patch fixes this issue by setting up local_specializations sooner in
> > tsubst_pack_expansion, before the handling of the unsubstituted_packs case.
> > It also adds a new policy to local_specialization_stack so that we could use
> > the
> > class here to conditionally replace local_specializations.
> >
> > Passes 'make check-c++', does this look OK to commit after
> > bootstrap/regtesting?
> >
> > gcc/cp/ChangeLog:
> >
> > PR c++/94628
> > * cp-tree.h (lss_policy::lss_inherit): New enumerator.
> > * pt.c (local_specialization_stack::local_specialization_stack):
> > Handle
> > an lss_inherit policy.
> > (local_specialization_stack::~local_specialization_stack): Likewise.
> > (tsubst_pack_expansion): Use a local_specialization_stack instead of
> > manually saving and restoring local_specializations. Conditionally
> > replace local_specializations sooner, before the handling of the
> > unsubstituted_packs case.
> >
> > gcc/testsuite/ChangeLog:
> >
> > PR c++/94628
> > * g++.dg/cpp0x/variadic179.C: New test.
> > * g++.dg/cpp0x/variadic180.C: New test.
> > ---
> > gcc/cp/cp-tree.h | 2 +-
> > gcc/cp/pt.c | 38 +++++++++++-------------
> > gcc/testsuite/g++.dg/cpp0x/variadic179.C | 16 ++++++++++
> > gcc/testsuite/g++.dg/cpp0x/variadic180.C | 25 ++++++++++++++++
> > 4 files changed, 59 insertions(+), 22 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/cpp0x/variadic179.C
> > create mode 100644 gcc/testsuite/g++.dg/cpp0x/variadic180.C
> >
> > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> > index 000badc21ac..5b3b9507474 100644
> > --- a/gcc/cp/cp-tree.h
> > +++ b/gcc/cp/cp-tree.h
> > @@ -5424,7 +5424,7 @@ enum unification_kind_t {
> > // An RAII class used to create a new pointer map for local
> > // specializations. When the stack goes out of scope, the
> > // previous pointer map is restored.
> > -enum lss_policy { lss_blank, lss_copy };
> > +enum lss_policy { lss_blank, lss_copy, lss_inherit };
> > class local_specialization_stack
> > {
> > public:
> > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> > index 9e39f46a090..9cea663514d 100644
> > --- a/gcc/cp/pt.c
> > +++ b/gcc/cp/pt.c
> > @@ -83,7 +83,9 @@ static tree cur_stmt_expr;
> > local_specialization_stack::local_specialization_stack (lss_policy policy)
> > : saved (local_specializations)
> > {
> > - if (policy == lss_blank || !saved)
> > + if (policy == lss_inherit)
> > + ;
>
> Maybe assert that saved is non-null?
Hmm, it looks like adding an assert to that effect breaks substituting
into the TYPE_ARG_TYPES of a variadic function template, because
local_specializations is not set up until we finish substituting into
the decl and start instantiating the body.
>
> lss_inherit suggests to me that we could add things and then pop back to the
> previous state, which is the actual meaning of lss_copy. Maybe lss_nop or
> lss_same instead?
Sounds good, I will commit the patch with lss_inherit changed to
lss_nop shortly (and without the assert I suppose).
>
> OK with those changes.
>
> > + else if (policy == lss_blank || !saved)
> > local_specializations = new hash_map<tree, tree>;
> > else
> > local_specializations = new hash_map<tree, tree>(*saved);
> > @@ -91,8 +93,11 @@ local_specialization_stack::local_specialization_stack
> > (lss_policy policy)
> > local_specialization_stack::~local_specialization_stack ()
> > {
> > - delete local_specializations;
> > - local_specializations = saved;
> > + if (local_specializations != saved)
> > + {
> > + delete local_specializations;
> > + local_specializations = saved;
> > + }
> > }
> > /* True if we've recursed into fn_type_unification too many times. */
> > @@ -12694,7 +12699,6 @@ tsubst_pack_expansion (tree t, tree args,
> > tsubst_flags_t complain,
> > bool unsubstituted_fn_pack = false;
> > int i, len = -1;
> > tree result;
> > - hash_map<tree, tree> *saved_local_specializations = NULL;
> > bool need_local_specializations = false;
> > int levels;
> > @@ -12893,7 +12897,15 @@ tsubst_pack_expansion (tree t, tree args,
> > tsubst_flags_t complain,
> > = build_extra_args (pattern, args, complain);
> > return t;
> > }
> > - else if (unsubstituted_packs)
> > +
> > + /* If NEED_LOCAL_SPECIALIZATIONS then we're in a late-specified return
> > + type, so create our own local specializations map; the current map is
> > + either NULL or (in the case of recursive unification) might have
> > + bindings that we don't want to use or alter. */
> > + local_specialization_stack lss (need_local_specializations
> > + ? lss_blank : lss_inherit);
> > +
> > + if (unsubstituted_packs)
> > {
> > /* There were no real arguments, we're just replacing a parameter
> > pack with another version of itself. Substitute into the
> > @@ -12910,16 +12922,6 @@ tsubst_pack_expansion (tree t, tree args,
> > tsubst_flags_t complain,
> > gcc_assert (len >= 0);
> > - if (need_local_specializations)
> > - {
> > - /* We're in a late-specified return type, so create our own local
> > - specializations map; the current map is either NULL or (in the
> > - case of recursive unification) might have bindings that we don't
> > - want to use or alter. */
> > - saved_local_specializations = local_specializations;
> > - local_specializations = new hash_map<tree, tree>;
> > - }
> > -
> > /* For each argument in each argument pack, substitute into the
> > pattern. */
> > result = make_tree_vec (len);
> > @@ -12966,12 +12968,6 @@ tsubst_pack_expansion (tree t, tree args,
> > tsubst_flags_t complain,
> > }
> > }
> > - if (need_local_specializations)
> > - {
> > - delete local_specializations;
> > - local_specializations = saved_local_specializations;
> > - }
> > -
> > /* If the dependent pack arguments were such that we end up with only a
> > single pack expansion again, there's no need to keep it in a
> > TREE_VEC. */
> > if (len == 1 && TREE_CODE (result) == TREE_VEC
> > diff --git a/gcc/testsuite/g++.dg/cpp0x/variadic179.C
> > b/gcc/testsuite/g++.dg/cpp0x/variadic179.C
> > new file mode 100644
> > index 00000000000..f04d3f753ca
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp0x/variadic179.C
> > @@ -0,0 +1,16 @@
> > +// PR c++/94628
> > +// { dg-do compile { target c++11 } }
> > +
> > +int f(int, int);
> > +int f(int);
> > +
> > +template<class...Args>
> > +auto select(Args... args) -> decltype(f(args...))
> > +{
> > + if (sizeof...(Args) > 1)
> > + return select<char>(7);
> > + else
> > + return 0;
> > +}
> > +
> > +int a = select(0, 1);
> > diff --git a/gcc/testsuite/g++.dg/cpp0x/variadic180.C
> > b/gcc/testsuite/g++.dg/cpp0x/variadic180.C
> > new file mode 100644
> > index 00000000000..e8fcdd0a64d
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp0x/variadic180.C
> > @@ -0,0 +1,25 @@
> > +// PR c++/94628
> > +// A variant of variadic101.C where the recursive call to deref
> > +// has its first template argument explicitly given.
> > +// { dg-do compile { target c++11 } }
> > +
> > +template<class T>
> > +struct Container
> > +{ T f() const; };
> > +
> > +template<class T>
> > +T deref(const T& t)
> > +{ return t; }
> > +
> > +
> > +template <class T, class... Args>
> > +auto
> > +deref(const T& u, int r, Args... args)
> > +-> decltype(deref(u.f(), args...))
> > +{ return deref<decltype(u.f())>(u.f(), args...); }
> > +
> > +int main(void)
> > +{
> > + Container<Container<int>> v;
> > + deref(v,1,2);
> > +}
> >
>
>
