On Tue, 28 Apr 2020, Patrick Palka wrote:
> 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.
I've just created PR94830 for this issue.
>
> >
> > > +
> > > +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" }
> > >
> >
> >
>
