On Tue, 28 Apr 2020, Jason Merrill wrote:
> On 4/28/20 1:41 PM, Patrick Palka wrote:
> > On Tue, 28 Apr 2020, Patrick Palka wrote:
> >
> > > On Tue, 28 Apr 2020, Jason Merrill wrote:
> > > > On 4/28/20 9:48 AM, Patrick Palka wrote:
> > > > > When printing the substituted parameter list of a requires-expression
> > > > > as
> > > > > part of the "in requirements with ..." context line during concepts
> > > > > diagnostics, we weren't considering that substitution into a parameter
> > > > > pack can yield zero or multiple parameters.
> > > > >
> > > > > Since this patch affects only concepts diagnostics, so far I tested
> > > > > with
> > > > > RUNTESTFLAGS="dg.exp=*concepts*" and also verified that we no longer
> > > > > ICE
> > > > > with the unreduced testcase in the PR. Is this OK to commit after a
> > > > > full bootstrap and regtest?
> > > >
> > > > OK.
> > >
> > > Thanks for the review.
> > >
> > > >
> > > > Though I wonder about printing the dependent form and template arguments
> > > > instead. Do you have an opinion?
> > >
> > > I was going to say that on the one hand, it's convenient to have the
> > > substituted form printed since it would let us to see through
> > > complicated dependent type aliases, but it seems we don't strip type
> > > aliases when pretty printing a parameter and its type with '%q#D'
> > > anyway. And I can't think of any other possible advantage of printing
> > > the substituted form.
> > >
> > > So IMHO printing the dependent form and template arguments instead would
> > > be better here. I'll try to write a patch for this today.
> >
> > Like so, tested so far with RUNTESTFLAGS="dg.exp=*concepts*" and also
> > verified we no longer ICE with the unreduced testcase in the PR. Does
> > the following look OK to commit after a full bootstrap and regtest?
> >
> > -- >8 --
> >
> > Subject: [PATCH] c++: Parameter pack in requires parameter list [PR94808]
> >
> > When printing the substituted parameter list of a requires-expression as
> > part of the "in requirements with ..." context line during concepts
> > diagnostics, we weren't considering that substitution into a parameter
> > pack can yield zero or multiple parameters.
> >
> > This patch changes the way we print the parameter list of a
> > requires-expression in print_requires_expression_info. We now print the
> > dependent form of the parameter list (along with its template parameter
> > mapping) instead of printing its substituted form. Besides being an
> > improvement in its own, this also sidesteps the above parameter pack
> > expansion issue altogether.
> >
> > Tested with RUNTESTFLAGS="dg.exp=*concepts*" and also verified we longer
> > ICE with the unreduced testcase in the PR. Does this look OK to commit
> > after a bootstrap and regtest?
> >
> > gcc/cp/ChangeLog:
> >
> > PR c++/94808
> > * error.c (print_requires_expression_info): Print the dependent
> > form of the parameter list with its template parameter mapping,
> > rather than printing the substituted form.
> >
> > gcc/testsuite/ChangeLog:
> >
> > PR c++/94808
> > * g++.dg/concepts/diagnostic12.C: New test.
> > * g++.dg/concepts/diagnostic5.C: Adjust expected diagnostic.
> > ---
> > gcc/cp/error.c | 16 ++++------------
> > gcc/testsuite/g++.dg/concepts/diagnostic12.C | 16 ++++++++++++++++
> > gcc/testsuite/g++.dg/concepts/diagnostic5.C | 4 ++--
> > 3 files changed, 22 insertions(+), 14 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/concepts/diagnostic12.C
> >
> > diff --git a/gcc/cp/error.c b/gcc/cp/error.c
> > index 98c163db572..46970f9b699 100644
> > --- a/gcc/cp/error.c
> > +++ b/gcc/cp/error.c
> > @@ -3746,7 +3746,6 @@ print_requires_expression_info (diagnostic_context
> > *context, tree constr, tree a
> > map = tsubst_parameter_mapping (map, args, tf_none, NULL_TREE);
> > if (map == error_mark_node)
> > return;
> > - args = get_mapped_args (map);
> > print_location (context, cp_expr_loc_or_input_loc (expr));
> > pp_verbatim (context->printer, "in requirements ");
> > @@ -3756,19 +3755,12 @@ print_requires_expression_info (diagnostic_context
> > *context, tree constr, tree a
> > pp_verbatim (context->printer, "with ");
> > while (parms)
> > {
> > - tree next = TREE_CHAIN (parms);
> > -
> > - TREE_CHAIN (parms) = NULL_TREE;
> > - cp_unevaluated u;
> > - tree p = tsubst (parms, args, tf_none, NULL_TREE);
> > - pp_verbatim (context->printer, "%q#D", p);
> > - TREE_CHAIN (parms) = next;
> > -
> > - if (next)
> > + pp_verbatim (context->printer, "%q#D", parms);
> > + if (TREE_CHAIN (parms))
> > pp_separate_with_comma ((cxx_pretty_printer *)context->printer);
> > -
> > - parms = next;
> > + parms = TREE_CHAIN (parms);
> > }
> > + pp_cxx_parameter_mapping ((cxx_pretty_printer *)context->printer, map);
> > pp_verbatim (context->printer, "\n");
> > }
> > diff --git a/gcc/testsuite/g++.dg/concepts/diagnostic12.C
> > b/gcc/testsuite/g++.dg/concepts/diagnostic12.C
> > new file mode 100644
> > index 00000000000..a757342f754
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/concepts/diagnostic12.C
> > @@ -0,0 +1,16 @@
> > +// PR c++/94808
> > +// { dg-do compile { target concepts } }
> > +
> > +template<typename T, typename... Args>
> > + concept c1 = requires (T t, Args... args) { *t; };
> > +// { dg-message "in requirements with .T t., .Args ... args. .with.* Args =
> > \{\}" "" { target *-*-* } .-1 }
>
> Doesn't this print a binding for T?
Yes, but the order in which the binding for T and the binding for Args
is printed appears to be nondeterministic, so I lazily opted to check
for just one of the bindings.
This nondeterminism stems from how find_template_parameters is
implemented, which uses a hash_set<tree> to keep track of seen template
parameters and then builds a TREE_LIST of mappings according to the
(nondeterministic) order of entries in the table. I think it is
harmless semantically but it does affect diagnostics such as these.
>
> > +
> > +static_assert(c1<int>); // { dg-error "failed" }
> > +
> > +void f(...);
> > +
> > +template<typename... Args>
> > + concept c2 = requires (Args... args) { f(*args...); };
> > +// { dg-message "in requirements with .Args ... args. .with Args = \{int,
> > char\}" "" { target *-*-* } .-1 }
> > +
> > +static_assert(c2<int, char>); // { dg-error "failed" }
> > diff --git a/gcc/testsuite/g++.dg/concepts/diagnostic5.C
> > b/gcc/testsuite/g++.dg/concepts/diagnostic5.C
> > index 0d890d6f548..81705f6a0c6 100644
> > --- a/gcc/testsuite/g++.dg/concepts/diagnostic5.C
> > +++ b/gcc/testsuite/g++.dg/concepts/diagnostic5.C
> > @@ -9,7 +9,7 @@ template<typename T>
> > template<typename T>
> > concept c2 = requires (T x) { *x; };
> > // { dg-message "satisfaction of .c2<T>. .with T = char." "" { target
> > *-*-* } .-1 }
> > -// { dg-message "in requirements with .char x." "" { target *-*-* } .-2 }
> > +// { dg-message "in requirements with .T x. .with T = char." "" { target
> > *-*-* } .-2 }
> > // { dg-message "required expression .* is invalid" "" { target *-*-* }
> > .-3 }
> > template<typename T>
> > @@ -25,7 +25,7 @@ template<typename T>
> > template<typename T>
> > concept c5 = requires (T x) { { &x } -> c1; };
> > // { dg-message "satisfaction of .c5<T>. .with T = char." "" { target
> > *-*-* } .-1 }
> > -// { dg-message "in requirements with .char x." "" { target *-*-* } .-2 }
> > +// { dg-message "in requirements with .T x. .with T = char." "" { target
> > *-*-* } .-2 }
> > template<typename T>
> > requires (c1<T> || c2<T>) || (c3<T> || c4<T>) || c5<T> // { dg-message
> > "49: no operand" }
> >
>
>
