diff mbox series

c++: Fix hashing and testing for equality of ATOMIC_CONST_EXPRs

Message ID 20200212161557.3623035-1-ppalka@redhat.com
State New
Headers show
Series c++: Fix hashing and testing for equality of ATOMIC_CONST_EXPRs | expand

Commit Message

Patrick Palka Feb. 12, 2020, 4:15 p.m. UTC
Two equal atomic constraint expressions do not necessarily share the same tree,
so we can't assume that two ATOMIC_CONST_EXPRs are equal if and only if they
point to the same tree.  The main consequence of this invalid assumption is that
the constraint subsumption checker may reject a valid partial specialization for
a constrained template because the checker decides that the constraints on the
partial specialization do not subsume the constraints on the primary template,
as in the test case below.  Another consequence is that it probably makes the
constraint caches less effective.

This patch changes atomic_constraints_identical_p to test for equality of
ATOMIC_CONST_EXPRs using cp_tree_equal, and changes hash_atomic_constraint to
hash the ATOMIC_CONST_EXPR with iterative_hash_template_arg.  Originally I used
template_args_equal but cp_tree_equal seems to suffice.

Does this look like an OK solution to this issue?  Are the testcase adjustments
valid?

gcc/cp/ChangeLog:

	* constraint.cc (atomic_constraints_identical_p): Use cp_tree_equal to
	test for equality between ATOMIC_CONST_EXPRs.
	(hash_atomic_constraint): Use iterative_hash_template to hash the
	ATOMIC_CONST_EXPR.

gcc/testsuite/ChangeLog:

	* g++.dg/concepts/variadic4.C: Adjust test to no longere expect an
	error.
	* g++.dg/cpp2a/concepts-pr84551.C: Likewise.
	* g++.dg/cpp2a/concepts-partial-spec7.C: New test.
---
 gcc/cp/constraint.cc                          |  4 +-
 gcc/testsuite/g++.dg/concepts/variadic4.C     |  4 +-
 .../g++.dg/cpp2a/concepts-partial-spec7.C     | 40 +++++++++++++++++++
 gcc/testsuite/g++.dg/cpp2a/concepts-pr84551.C |  2 +-
 4 files changed, 44 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-partial-spec7.C

Comments

Jason Merrill Feb. 13, 2020, 11:10 p.m. UTC | #1
On 2/12/20 5:15 PM, Patrick Palka wrote:
> Two equal atomic constraint expressions do not necessarily share the same tree,
> so we can't assume that two ATOMIC_CONST_EXPRs are equal if and only if they
> point to the same tree.

This is incorrect; comparison of atomic constraints is based on them 
coming from the same actual tokens, which is why we use pointer comparison.

In your concepts-partial-spec7.C, test2 is properly rejected.  If you 
want to be able to write the same constraint in multiple places, you 
need to use a concept.

13.5.1.2:

Two atomic constraints are identical if they are formed from the same 
expression and the targets of the parameter mappings are equivalent 
according to the rules for expressions described in 13.7.6.1.

Jason
Patrick Palka Feb. 13, 2020, 11:45 p.m. UTC | #2
On Fri, 14 Feb 2020, Jason Merrill wrote:

> On 2/12/20 5:15 PM, Patrick Palka wrote:
> > Two equal atomic constraint expressions do not necessarily share the same
> > tree,
> > so we can't assume that two ATOMIC_CONST_EXPRs are equal if and only if they
> > point to the same tree.
> 
> This is incorrect; comparison of atomic constraints is based on them coming
> from the same actual tokens, which is why we use pointer comparison.
> 
> In your concepts-partial-spec7.C, test2 is properly rejected.  If you want to
> be able to write the same constraint in multiple places, you need to use a
> concept.
> 
> 13.5.1.2:
> 
> Two atomic constraints are identical if they are formed from the same
> expression and the targets of the parameter mappings are equivalent according
> to the rules for expressions described in 13.7.6.1.

I see, that makes complete sense now.  I was stumbling over what the
spec meant by "the same expression."  Thanks for clearing up my
misunderstanding.
diff mbox series

Patch

diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
index 58044cd0f9d..935c3556272 100644
--- a/gcc/cp/constraint.cc
+++ b/gcc/cp/constraint.cc
@@ -933,7 +933,7 @@  atomic_constraints_identical_p (tree t1, tree t2)
   gcc_assert (TREE_CODE (t1) == ATOMIC_CONSTR);
   gcc_assert (TREE_CODE (t2) == ATOMIC_CONSTR);
 
-  if (ATOMIC_CONSTR_EXPR (t1) != ATOMIC_CONSTR_EXPR (t2))
+  if (!cp_tree_equal (ATOMIC_CONSTR_EXPR (t1), ATOMIC_CONSTR_EXPR (t2)))
     return false;
 
   if (!parameter_mapping_equivalent_p (t1, t2))
@@ -981,7 +981,7 @@  hash_atomic_constraint (tree t)
   gcc_assert (TREE_CODE (t) == ATOMIC_CONSTR);
 
   /* Hash the identity of the expression.  */
-  hashval_t val = htab_hash_pointer (ATOMIC_CONSTR_EXPR (t));
+  hashval_t val = iterative_hash_template_arg (ATOMIC_CONSTR_EXPR (t), 0);
 
   /* Hash the targets of the parameter map.  */
   tree p = ATOMIC_CONSTR_MAP (t);
diff --git a/gcc/testsuite/g++.dg/concepts/variadic4.C b/gcc/testsuite/g++.dg/concepts/variadic4.C
index d6eea49b958..b073bcce5e9 100644
--- a/gcc/testsuite/g++.dg/concepts/variadic4.C
+++ b/gcc/testsuite/g++.dg/concepts/variadic4.C
@@ -12,9 +12,7 @@  struct zip;
 
 template<Sequence... Seqs>
     requires requires { typename list<Seqs...>; } // && (Sequence<Seqs> && ...)
-struct zip<Seqs...> {}; // { dg-error "does not specialize" }
-// The constraints of the specialization and the sequence are not
-// comparable; the specializations are unordered.
+struct zip<Seqs...> {};
 
 int main()
 {
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-partial-spec7.C b/gcc/testsuite/g++.dg/cpp2a/concepts-partial-spec7.C
new file mode 100644
index 00000000000..ed77ed4d362
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-partial-spec7.C
@@ -0,0 +1,40 @@ 
+// { dg-do compile { target c++2a } }
+
+template<typename T>
+  inline constexpr bool foo = true;
+
+template<typename T>
+  inline constexpr bool bar = true;
+
+// The following two partial specializations are both valid, but the constraint
+// subsumption checker used to compare ATOMIC_CONSTR_EXPRs by comparing their
+// pointers, leading to it accepting the partial specialization of test1 and
+// rejecting the partial specialization of test2: in the test1 case, the
+// TEMPLATE_ID_EXPR of foo<T> in the constraints was shared between the primary
+// template and partial specialization, and in the test2 case the
+// TEMPLATE_ID_EXPRs of foo<T> in the constraints were different (but equal)
+// trees.
+
+template<typename T>
+  concept baz = foo<T>;
+
+template<typename T>
+  requires baz<T>
+  struct test1
+  { };
+
+template<typename T>
+  requires baz<T> && bar<T>
+  struct test1<T>
+  { };
+
+template<typename T>
+  requires foo<T>
+  struct test2
+  { };
+
+template<typename T>
+  requires foo<T> && bar<T>
+  struct test2<T>
+  { };
+
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-pr84551.C b/gcc/testsuite/g++.dg/cpp2a/concepts-pr84551.C
index e40796f4975..e7498437178 100644
--- a/gcc/testsuite/g++.dg/cpp2a/concepts-pr84551.C
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-pr84551.C
@@ -8,4 +8,4 @@  template<template<typename T> requires C<T> class TT> struct A {};
 
 template<typename U> requires true struct B {};
 
-A<B> a;				// { dg-error "" }
+A<B> a;