On Fri, 6 Nov 2020, Jason Merrill wrote:
> On 11/5/20 8:40 PM, Patrick Palka wrote:
> > This patch (naively) extends the PR93907 fix to also apply to variadic
> > concepts invoked with a type argument pack. Without this, we ICE on
> > the below testcase (a variadic version of concepts-using2.C) in the same
> > manner as we used to on concepts-using2.C before r10-7133.
> >
> > Patch series bootstrapped and regtested on x86_64-pc-linux-gnu,
> > and also tested against cmcstl2 and range-v3.
> >
> > gcc/cp/ChangeLog:
> >
> > PR c++/93907
> > * constraint.cc (tsubst_parameter_mapping): Also canonicalize
> > the type arguments of a TYPE_ARGUMENT_PACk.
> >
> > gcc/testsuite/ChangeLog:
> >
> > PR c++/93907
> > * g++.dg/cpp2a/concepts-using3.C: New test, based off of
> > concepts-using2.C.
> > ---
> > gcc/cp/constraint.cc | 10 ++++
> > gcc/testsuite/g++.dg/cpp2a/concepts-using3.C | 52 ++++++++++++++++++++
> > 2 files changed, 62 insertions(+)
> > create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-using3.C
> >
> > diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
> > index b6f6f0d02a5..c871a8ab86a 100644
> > --- a/gcc/cp/constraint.cc
> > +++ b/gcc/cp/constraint.cc
> > @@ -2252,6 +2252,16 @@ tsubst_parameter_mapping (tree map, tree args,
> > subst_info info)
>
> Hmm, the
>
> > else if (ARGUMENT_PACK_P (arg))
> > new_arg = tsubst_argument_pack (arg, args, complain, in_decl);
>
> just above this seems redundant, since tsubst_template_arg handles packs just
> fine. In fact, I wonder why tsubst_argument_pack is used specifically
> anywhere? It seems to get some edge cases better than the code in tsubst, but
> the solution to that would seem to be replacing the code in tsubst with a call
> to tsubst_argument_pack; then we can remove all the other calls to the
> function.
They seem interchangeable here wrt handling TYPE_ARGUMENT_PACKs, but not
NONTYPE_ARGUMENT_PACKs. It looks like tsubst_template_arg ends up just
issuing an error from tsubst_expr if we try using it to substitute into
a NONTYPE_ARGUMENT_PACK.
>
> > new_arg = tsubst_template_arg (arg, args, complain, in_decl);
> > if (TYPE_P (new_arg))
> > new_arg = canonicalize_type_argument (new_arg, complain);
> > + if (TREE_CODE (new_arg) == TYPE_ARGUMENT_PACK)
> > + {
> > + tree pack_args = ARGUMENT_PACK_ARGS (new_arg);
> > + for (int i = 0; i < TREE_VEC_LENGTH (pack_args); i++)
> > + {
> > + tree& pack_arg = TREE_VEC_ELT (pack_args, i);
> > + if (TYPE_P (pack_arg))
> > + pack_arg = canonicalize_type_argument (pack_arg,
> > complain);
>
> Do we need the TYPE_P here, since we already know we're in a
> TYPE_ARGUMENT_PACK? That is, can an element of a TYPE_ARGUMENT_PACK be an
> invalid argument to canonicalize_type_argument?
With -fconcepts-ts, the elements of a TYPE_ARGUMENT_PACK here can be
TEMPLATE_DECLs, as in e.g. line 28 of concepts/template-parm3.C.
>
> OTOH, I wonder if we need to canonicalize non-type arguments here as well?
Hmm, I'm not sure. Not doing so should at worst result in a
satisfaction cache miss in release builds, and in checking builds should
get caught by the hash table sanitizer. I haven't been able to come up
with a testcase that demonstrates it's necessary.
>
> I wonder if tsubst_template_arg should canonicalize rather than leave that up
> to the caller? I suppose that could do a bit more work when the result is
> going to end up in convert_template_argument and get canonicalized again; I
> don't know if that would be significant.
That seems like it works, based on some limited testing. But there are
only two users of canonicalize_template_argument outside of
convert_template_argument itself, and the one use in unify is still
needed even with this change (or else we get many ICEs coming from
verify_unstripped_args if we try to remove it). So the benefit of such
a change seems marginal at the moment.
>
> > }
> > if (new_arg == error_mark_node)
> > return error_mark_node;
> > diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-using3.C
> > b/gcc/testsuite/g++.dg/cpp2a/concepts-using3.C
> > new file mode 100644
> > index 00000000000..2c8ad40d104
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-using3.C
> > @@ -0,0 +1,52 @@
> > +// PR c++/93907
> > +// { dg-options -std=gnu++20 }
> > +
> > +// This testcase is a variadic version of concepts-using2.C; the only
> > +// difference is that 'cd' and 'ce' are now variadic concepts.
> > +
> > +template <int a> struct c {
> > + static constexpr int d = a;
> > + typedef c e;
> > +};
> > +template <typename> struct f;
> > +template <typename b> using g = typename f<b>::e;
> > +struct b;
> > +template <typename b> struct f { using e = b; };
> > +template <typename ai> struct m { typedef g<ai> aj; };
> > +template <typename b> struct n { typedef typename m<b>::aj e; };
> > +template <typename b> using an = typename n<b>::e;
> > +template <typename> constexpr bool ao = c<true>::d;
> > +template <typename> constexpr bool i = c<1>::d;
> > +template <typename> concept bb = i<b>;
> > +#ifdef __SIZEOF_INT128__
> > +using cc = __int128;
> > +#else
> > +using cc = long long;
> > +#endif
> > +template <typename...> concept cd = bb<cc>;
> > +template <typename... bt> concept ce = requires { requires cd<bt...>; };
> > +template <typename bt> concept h = ce<bt>;
> > +template <typename bt> concept l = h<bt>;
> > +template <typename> concept cl = ao<b>;
> > +template <typename b> concept cp = requires(b j) {
> > + requires h<an<decltype(j.begin())>>;
> > +};
> > +struct o {
> > + template <cl b> requires cp<b> auto operator()(b) {}
> > +};
> > +template <typename b> using cm = decltype(o{}(b()));
> > +template <typename bt> concept ct = l<bt>;
> > +template <typename da> concept dd = ct<cm<da>>;
> > +template <typename da> concept de = dd<da>;
> > +struct {
> > + template <de da, typename b> void operator()(da, b);
> > +} di;
> > +struct p {
> > + void begin();
> > +};
> > +template <typename> using df = p;
> > +template <int> void q() {
> > + df<int> k;
> > + int d;
> > + di(k, d);
> > +}
> >
>
>
