On Tue, 28 Apr 2020, Jason Merrill wrote:
> On 4/28/20 3:19 PM, Patrick Palka wrote:
> > 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.
>
> Thanks. I'd think we should build up the list in find_template_parameter_info
> and only use the hash_set to avoid adding the same parameter more than once.
>
> This patch is OK.
Nice, building up the list in find_template_parameter_info works pretty
well. FWIW, here's a patch for this:
-- >8 --
Subject: [PATCH] c++: Nondeterministic concepts diagnostics [PR94830]
This patch makes the order in which template parameters appear in the
TREE_LIST returned by find_template_parameters deterministic between
runs.
The current nondeterminism is semantically harmless, but it has the
undesirable effect of causing some concepts diagnostics which print a
constraint's parameter mapping via pp_cxx_parameter_mapping to also be
nondeterministic, as in the testcases below.
Passes 'make check-c++', does this look OK to commit after a full
bootstrap and regtest?
(I considered reversing the resulting TREE_LIST in
find_template_parameters so that it's ordered with respect to when
keep_template_parm first sees each parameter, but doing so doesn't
always give the natural ordering anyway, since walk_tree_1 visits the
the template arguments of a TEMPLATE_ID_EXPR (i.e. the elements of a
TREE_VEC) backwards.)
gcc/cp/ChangeLog:
PR c++/94830
* pt.c (find_template_parameter_info::parm_list): New field.
(keep_template_parm): Use the new field to build up the
parameter list here instead of ...
(find_template_parameters): ... here. Return ftpi.parm_list.
gcc/testsuite/ChangeLog:
PR c++/94830
* g++.dg/concepts/diagnostics12.C: Clarify the dg-message now
that the corresponding diagnostic is deterministic.
* g++.dg/concepts/diagnostics13.C: New test.
---
gcc/cp/pt.c | 13 ++++++-------
gcc/testsuite/g++.dg/concepts/diagnostic12.C | 2 +-
gcc/testsuite/g++.dg/concepts/diagnostic13.C | 14 ++++++++++++++
3 files changed, 21 insertions(+), 8 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/concepts/diagnostic13.C
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index ba22d9ec538..94557a0439e 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -10440,11 +10440,13 @@ struct find_template_parameter_info
{
explicit find_template_parameter_info (tree ctx_parms)
: ctx_parms (ctx_parms),
- max_depth (TMPL_PARMS_DEPTH (ctx_parms))
+ max_depth (TMPL_PARMS_DEPTH (ctx_parms)),
+ parm_list (NULL_TREE)
{}
hash_set<tree> visited;
hash_set<tree> parms;
+ tree parm_list;
tree ctx_parms;
int max_depth;
};
@@ -10476,7 +10478,8 @@ keep_template_parm (tree t, void* data)
T and const T. Adjust types to their unqualified versions. */
if (TYPE_P (t))
t = TYPE_MAIN_VARIANT (t);
- ftpi->parms.add (t);
+ if (!ftpi->parms.add (t))
+ ftpi->parm_list = tree_cons (NULL_TREE, t, ftpi->parm_list);
return 0;
}
@@ -10587,11 +10590,7 @@ find_template_parameters (tree t, tree ctx_parms)
find_template_parameter_info ftpi (ctx_parms);
for_each_template_parm (t, keep_template_parm, &ftpi, &ftpi.visited,
/*include_nondeduced*/true, any_template_parm_r);
- tree list = NULL_TREE;
- for (hash_set<tree>::iterator iter = ftpi.parms.begin();
- iter != ftpi.parms.end(); ++iter)
- list = tree_cons (NULL_TREE, *iter, list);
- return list;
+ return ftpi.parm_list;
}
/* Returns true if T depends on any template parameter. */
diff --git a/gcc/testsuite/g++.dg/concepts/diagnostic12.C
b/gcc/testsuite/g++.dg/concepts/diagnostic12.C
index a757342f754..548ba9c1b3d 100644
--- a/gcc/testsuite/g++.dg/concepts/diagnostic12.C
+++ b/gcc/testsuite/g++.dg/concepts/diagnostic12.C
@@ -3,7 +3,7 @@
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 }
+// { dg-message "in requirements with .T t., .Args ... args. .with Args =
\{\}; T = int" "" { target *-*-* } .-1 }
static_assert(c1<int>); // { dg-error "failed" }
diff --git a/gcc/testsuite/g++.dg/concepts/diagnostic13.C
b/gcc/testsuite/g++.dg/concepts/diagnostic13.C
new file mode 100644
index 00000000000..accd8a6d2bd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/concepts/diagnostic13.C
@@ -0,0 +1,14 @@
+// PR c++/94830
+// { dg-do compile { target concepts } }
+
+template<typename T, typename R>
+ concept c = __is_same(T, R); // { dg-message "with T = int; R = char" }
+
+template<typename T, typename R>
+ requires c<T,R>
+void foo() { }
+
+void bar()
+{
+ foo<int, char>(); // { dg-error "unsatisfied constraints" }
+}
--
2.26.2.357.g86ab15cb15
