Re: [PATCH] c++: Incomplete parameter mappings during normalization
On Mon, 28 Sep 2020, Jason Merrill wrote: > On 9/25/20 4:44 PM, Patrick Palka wrote: > > In the testcase concepts7.C below, we currently reject the call to f1 > > but we accept the call to f2, even though their associated constraints > > are functionally equivalent. > > Rather, because they are not functionally equivalent. > > > The reason satisfaction differs for (!!C > C is due to normalization: the former is already an > > atom, and the latter is not. Normalization of the former yields itself, > > whereas normalization of the latter yields the atom 'true' with an empty > > parameter mapping (since the atom uses no template parameters). > > Yes. > > > So when > > building the latter atom we threw away the T::type term that would later > > result in substitution failure during satisfaction. > > > > However, [temp.constr.normal]/1 says: > > > >- ... > >- The normal form of a concept-id C is the normal > > form of the constraint-expression of C, after substituting A1, A2, > > ..., An for C's respective template parameters in the parameter > > mappings in each atomic constraint. > >- The normal form of any other expression E is the atomic constraint > > whose expression is E and whose parameter mapping is the identity > > mapping. > > > > I believe these two bullet points imply that the atom 'true' in the > > normal form of C should have the mapping R |-> T::type > > instead of the empty mapping that we give it, because according to the > > last bullet point, each atom should start out with the identity mapping > > that includes all template parameters. > > But [temp.constr.atomic] says, > > An atomic constraint is formed from an expression E and a mapping from the > template parameters that appear within E... I see.. Well, that certainly settles that :) I definitely read that passage more than once, but I must have glossed over this key wording each time. That, and my misinterpreting of "the identity mapping" as "the identity mapping over all template parameters" ultimately led me down the wrong path. > > So the identity mapping only includes template parameters that appear within > the expression, which in this case is none. > > Also, the discussion of the example in [temp.constr.normal] says, > > "Normalization of B’s constraint-expression is valid and results in T::value > (with the mapping T → U*) V true (with an empty mapping)." > > So I think the testcase is already handled as specified. Got it. Thanks for your clarification, and sorry for the noise. I'll make sure to read the standard ever more carefully next time. > > > This patch fixes this issue by always giving the first atom in the > > normal form of each concept a 'complete' parameter mapping, i.e. one > > that includes all template parameters. I think it suffices to do this > > only for the first atom so that we catch substitution failures like in > > concepts7.C at the right time. For the other atoms, their mappings can > > continue to include only template parameters used in the atom. > > > > I noticed that PR92268 alludes to this issue, so this patch refers to > > that PR and adds the PR's first testcase which we now accept. > > > > Is the above interpretation of the standard correct here? If so, does > > this seem like a good approach? > > > > gcc/cp/ChangeLog: > > > > PR c++/92268 > > * constraint.cc (build_parameter_mapping): Add a bool parameter > > 'complete'. When 'complete' is true, then include all in-scope > > template parameters in the mapping. > > (norm_info::update_context): Pass false as the 'complete' > > argument to build_parameter_mapping. > > (norm_info::normalized_first_atom_p): New bool data member. > > (normalize_logical_operation): Set info.normalized_first_atom_p > > after normalizing the left operand. > > (normalize_concept_check): Reset info.normalized_first_atom_p > > before normalizing this concept. > > (normalize_atom): Always give the first atom of a concept > > definition a complete parameter mapping. > > > > gcc/testsuite/ChangeLog: > > > > PR c++/92268 > > * g++.dg/cpp2a/concepts-pr92268.C: New test. > > * g++.dg/cpp2a/concepts-return-req1.C: Don't expect an error, > > as the call is no longer ambiguous. > > * g++.dg/cpp2a/concepts7.C: New test. > > * g++.dg/cpp2a/concepts8.C: New test. > > --- > > gcc/cp/constraint.cc | 44 --- > > gcc/testsuite/g++.dg/cpp2a/concepts-pr92268.C | 42 ++ > > .../g++.dg/cpp2a/concepts-return-req1.C | 2 +- > > gcc/testsuite/g++.dg/cpp2a/concepts7.C| 9 > > gcc/testsuite/g++.dg/cpp2a/concepts8.C| 25 +++ > > 5 files changed, 116 insertions(+), 6 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-pr92268.C > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts7.C > > create mode 100644 gcc/testsuite/g++.dg
Re: [PATCH] c++: Incomplete parameter mappings during normalization
On 9/25/20 4:44 PM, Patrick Palka wrote:
In the testcase concepts7.C below, we currently reject the call to f1
but we accept the call to f2, even though their associated constraints
are functionally equivalent.
Rather, because they are not functionally equivalent.
The reason satisfaction differs for (!!C is due to normalization: the former is already an
atom, and the latter is not. Normalization of the former yields itself,
whereas normalization of the latter yields the atom 'true' with an empty
parameter mapping (since the atom uses no template parameters).
Yes.
So when
building the latter atom we threw away the T::type term that would later
result in substitution failure during satisfaction.
However, [temp.constr.normal]/1 says:
- ...
- The normal form of a concept-id C is the normal
form of the constraint-expression of C, after substituting A1, A2,
..., An for C's respective template parameters in the parameter
mappings in each atomic constraint.
- The normal form of any other expression E is the atomic constraint
whose expression is E and whose parameter mapping is the identity
mapping.
I believe these two bullet points imply that the atom 'true' in the
normal form of C should have the mapping R |-> T::type
instead of the empty mapping that we give it, because according to the
last bullet point, each atom should start out with the identity mapping
that includes all template parameters.
But [temp.constr.atomic] says,
An atomic constraint is formed from an expression E and a mapping from
the template parameters that appear within E...
So the identity mapping only includes template parameters that appear
within the expression, which in this case is none.
Also, the discussion of the example in [temp.constr.normal] says,
"Normalization of B’s constraint-expression is valid and results in
T::value (with the mapping T → U*) V true (with an empty mapping)."
So I think the testcase is already handled as specified.
This patch fixes this issue by always giving the first atom in the
normal form of each concept a 'complete' parameter mapping, i.e. one
that includes all template parameters. I think it suffices to do this
only for the first atom so that we catch substitution failures like in
concepts7.C at the right time. For the other atoms, their mappings can
continue to include only template parameters used in the atom.
I noticed that PR92268 alludes to this issue, so this patch refers to
that PR and adds the PR's first testcase which we now accept.
Is the above interpretation of the standard correct here? If so, does
this seem like a good approach?
gcc/cp/ChangeLog:
PR c++/92268
* constraint.cc (build_parameter_mapping): Add a bool parameter
'complete'. When 'complete' is true, then include all in-scope
template parameters in the mapping.
(norm_info::update_context): Pass false as the 'complete'
argument to build_parameter_mapping.
(norm_info::normalized_first_atom_p): New bool data member.
(normalize_logical_operation): Set info.normalized_first_atom_p
after normalizing the left operand.
(normalize_concept_check): Reset info.normalized_first_atom_p
before normalizing this concept.
(normalize_atom): Always give the first atom of a concept
definition a complete parameter mapping.
gcc/testsuite/ChangeLog:
PR c++/92268
* g++.dg/cpp2a/concepts-pr92268.C: New test.
* g++.dg/cpp2a/concepts-return-req1.C: Don't expect an error,
as the call is no longer ambiguous.
* g++.dg/cpp2a/concepts7.C: New test.
* g++.dg/cpp2a/concepts8.C: New test.
---
gcc/cp/constraint.cc | 44 ---
gcc/testsuite/g++.dg/cpp2a/concepts-pr92268.C | 42 ++
.../g++.dg/cpp2a/concepts-return-req1.C | 2 +-
gcc/testsuite/g++.dg/cpp2a/concepts7.C| 9
gcc/testsuite/g++.dg/cpp2a/concepts8.C| 25 +++
5 files changed, 116 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-pr92268.C
create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts7.C
create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts8.C
diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
index d49957a6c4a..729d02b73d7 100644
--- a/gcc/cp/constraint.cc
+++ b/gcc/cp/constraint.cc
@@ -559,10 +559,14 @@ map_arguments (tree parms, tree args)
return parms;
}
-/* Build the parameter mapping for EXPR using ARGS. */
+/* Build the parameter mapping for EXPR using ARGS.
+
+ If COMPLETE then return the complete parameter mapping that includes
+ all in-scope template parameters. Otherwise include only the
+ parameters used by EXPR. */
static tree
-build_parameter_mapping (tree expr, tree args, tree decl)
+build_parameter_mapping (tree expr, tree args, tree decl, bool complete)
{
tree ctx_parms = NULL_TREE;
if (dec
