On 4/29/20 12:05 AM, Patrick Palka wrote:
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:
OK, thanks.
-- >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" }
+}
