public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Fix PR c++/8171: Comparing pointer-to-member-functions of  derived classes
@ 2007-10-30  9:23 Volker Reichelt
  2007-11-12  2:49 ` Mark Mitchell
  0 siblings, 1 reply; 6+ messages in thread
From: Volker Reichelt @ 2007-10-30  9:23 UTC (permalink / raw)
  To: gcc-patches

Hi,

the C++ frontend rejects comparisons (== or !=) between
pointer-to-member-functions where the involved classes are not
identical, but derived from each other like in this code snippet:

  struct A
  {
      void foo() {}
  };

  struct B : A {};

  int bar()
  {
      void (A::*pa)() = &A::foo;
      void (B::*pb)() = &B::foo;

      return pa == pb;
  }

The following patch fixes that by changing the case METHOD_TYPE in
in comptypes (from cp/typeck.c) to honor the flag STRICT when comparing
the first arguments of the functions. That's where the pointer to the
classes are stored. Actually the patch strips away the pointers (by
calling TREE_TYPE) in order to compare the underlying classes directly.

The function build_binary_op (also from cp/typeck.c) makes use of
this modified comparison for METHOD_TYPE by calling comptypes with
the parameter STRICT = COMPARE_BASE | COMPARE_DERIVED and not with
parameter STRICT = 0 (like the macro same_type_p does).
Instead of calling comptypes with the pointer-to-member-functions,
the pointers are stripped away before the call, so that the
member-functions are compared directly. If necessary, the suitable
delta offset when comparing member-functions of derived classes
is taken into account

The patch also adds to testcases: The first not only checks that
these comparisons are now accepted, but also checks that their
outcome is correct. The class hierarchy is chosen such the the delta
offset of the classes is non-zero. The second testcase checks that
comparisons where the underlying classes are not derived from each
other are rejected.

Bootstrapped and regtested on i686-pc-linux-gnu.
Ok for mainline?

Regards,
Volker

:ADDPATCH C++:


2007-10-29  Volker Reichelt  <v.reichelt@netcologne.de>

	PR c++/8171
	* typeck.c (comptypes) <METHOD_TYPE>: Do not fall-through to
	FUNCTION_TYPE, but use a slightly modified version: Compare the
	class-pointers according to the parameter STRICT.
	(build_binary_op) <EQ_EXPR, NE_EXPR>: Allow derived classes when
	comparing ptr-to-member-functions.  Strip pointer before calling
	comptypes.  Add delta offset if necessary.

===================================================================
--- gcc/gcc/cp/typeck.c	2006-03-14 18:36:29 +0100
+++ gcc/gcc/cp/typeck.c	2006-04-21 01:45:33 +0200
@@ -1028,6 +1028,16 @@ comptypes (tree t1, tree t2, int strict)
       break;
 
     case METHOD_TYPE:
+      if (!same_type_p (TREE_TYPE (t1), TREE_TYPE (t2)))
+	return false;
+      if (!comptypes (TREE_TYPE (TREE_VALUE (TYPE_ARG_TYPES (t1))),
+		      TREE_TYPE (TREE_VALUE (TYPE_ARG_TYPES (t2))), strict))
+	return false;
+      if (!compparms (TREE_CHAIN (TYPE_ARG_TYPES (t1)),
+		      TREE_CHAIN (TYPE_ARG_TYPES (t2))))
+	return false;
+      break;
+
     case FUNCTION_TYPE:
       if (!same_type_p (TREE_TYPE (t1), TREE_TYPE (t2)))
 	return false;
@@ -3393,7 +3403,9 @@ build_binary_op (enum tree_code code,
       else if (TYPE_PTRMEMFUNC_P (type1) && null_ptr_cst_p (op0))
 	return cp_build_binary_op (code, op1, op0);
       else if (TYPE_PTRMEMFUNC_P (type0) && TYPE_PTRMEMFUNC_P (type1)
-	       && same_type_p (type0, type1))
+	       && comptypes (TREE_TYPE (TYPE_PTRMEMFUNC_FN_TYPE (type0)),
+			     TREE_TYPE (TYPE_PTRMEMFUNC_FN_TYPE (type1)),
+			     COMPARE_BASE | COMPARE_DERIVED))
 	{
 	  /* E will be the final comparison.  */
 	  tree e;
@@ -3422,13 +3434,25 @@ build_binary_op (enum tree_code code, tr
 	      /* We generate:
 
 		 (op0.pfn == op1.pfn
-		  && ((op0.delta == op1.delta)
-     		       || (!op0.pfn && op0.delta & 1 == 0 
+		  && ((op0.delta + n == op1.delta)
+		       || (!op0.pfn && op0.delta & 1 == 0
 			   && op1.delta & 1 == 0))
 
-	         The reason for the `!op0.pfn' bit is that a NULL
-	         pointer-to-member is any member with a zero PFN and
-	         LSB of the DELTA field is 0.  */
+		 N is the DELTA offset between the involved classes.
+		 The reason for the `!op0.pfn' bit is that a NULL
+		 pointer-to-member is any member with a zero PFN and
+		 LSB of the DELTA field is 0.  */
+
+	      e1 = get_delta_difference (TYPE_PTRMEMFUNC_OBJECT_TYPE (type0),
+					 TYPE_PTRMEMFUNC_OBJECT_TYPE (type1),
+					 /*force=*/true, /*c_cast_p=*/0);
+	      if (!integer_zerop (e1))
+		{
+		  /* Adjust offset.  */
+		  e1 = cp_build_binary_op (LSHIFT_EXPR, e1, integer_one_node);
+		  /* Compute op0.delta + n.  */
+		  delta0 = cp_build_binary_op (PLUS_EXPR, delta0, e1);
+		}
 
 	      e1 = cp_build_binary_op (BIT_AND_EXPR,
 				       delta0, 
@@ -3451,12 +3475,19 @@ build_binary_op (enum tree_code code, tr
 	    {
 	      /* We generate:
 
-	         (op0.pfn == op1.pfn
-	         && (!op0.pfn || op0.delta == op1.delta))
+		 (op0.pfn == op1.pfn
+		  && (!op0.pfn || op0.delta + n == op1.delta))
 
-	         The reason for the `!op0.pfn' bit is that a NULL
-	         pointer-to-member is any member with a zero PFN; the
-	         DELTA field is unspecified.  */
+		 N is the DELTA offset between the involved classes.
+		 The reason for the `!op0.pfn' bit is that a NULL
+		 pointer-to-member is any member with a zero PFN; the
+		 DELTA field is unspecified.  */
+
+	      e1 = get_delta_difference (TYPE_PTRMEMFUNC_OBJECT_TYPE (type0),
+					 TYPE_PTRMEMFUNC_OBJECT_TYPE (type1),
+					 /*force=*/true, /*c_cast_p=*/0);
+	      if (!integer_zerop (e1))
+		delta0 = cp_build_binary_op (PLUS_EXPR, delta0, e1);
  
     	      e1 = cp_build_binary_op (EQ_EXPR, delta0, delta1);
 	      e2 = cp_build_binary_op (EQ_EXPR,
===================================================================

2007-10-29  Volker Reichelt  <v.reichelt@netcologne.de>

	PR c++/8171
	* g++.dg/conversion/ptrmemfun1.C: New test.
	* g++.dg/conversion/ptrmemfun2.C: New test.

===================================================================
--- gcc/gcc/testsuite/g++.dg/conversion/ptrmemfun1.C	2005-08-29 00:25:44 +0200
+++ gcc/gcc/testsuite/g++.dg/conversion/ptrmemfun1.C	2006-04-21 17:17:12 +0200
@@ -0,0 +1,43 @@
+// PR c++/8171
+// Origin: <zyzstar@ibl.sk>
+// { dg-do run }
+
+extern "C" void abort();
+
+struct A
+{
+  int i;
+  void foo() {}
+  void bar() {}
+  void baz() {}
+};
+
+struct B
+{
+  int j;
+};
+
+struct C : B, A
+{
+  void baz() {}
+};
+
+int main()
+{
+  void (A::*pa)() = &A::foo;
+  void (C::*pb)() = &C::foo;
+  void (C::*pc)() = &C::bar;
+  void (A::*pd)() = &A::baz;
+  void (C::*pe)() = &C::baz;
+
+  if (pa != pb || pb != pa)
+    abort();
+  if (pa == pc || pc == pa)
+    abort();
+  if (pb == pc || pc == pb)
+    abort();
+  if (pd == pe || pe == pd)
+    abort();
+
+  return 0;
+}
===================================================================
--- gcc/gcc/testsuite/g++.dg/conversion/ptrmemfun2.C	2005-08-29 00:25:44 +0200
+++ gcc/gcc/testsuite/g++.dg/conversion/ptrmemfun2.C	2006-04-21 18:22:20 +0200
@@ -0,0 +1,19 @@
+// PR c++/8171
+// { dg-do compile }
+
+struct A
+{
+  void foo() {}
+};
+
+struct B : A {};
+
+struct C : A {};
+
+int main()
+{
+  void (B::*pb)() = &B::foo;
+  void (C::*pc)() = &C::foo;
+
+  return pb == pc;  // { dg-error "invalid operands" }
+}
===================================================================

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] Fix PR c++/8171: Comparing pointer-to-member-functions of   derived classes
@ 2007-11-27  6:19 Volker Reichelt
  0 siblings, 0 replies; 6+ messages in thread
From: Volker Reichelt @ 2007-11-27  6:19 UTC (permalink / raw)
  To: gcc-patches

Since my mailer screwed up the last message, I'm resending the message
below. Apologies, if you receive the message twice.

On 11 Nov, Mark Mitchell wrote:

Sorry for the long delay!

> Volker Reichelt wrote:
> 
>> :ADDPATCH C++:
>> 
>> 
>> 2007-10-29  Volker Reichelt  <v.reichelt@netcologne.de>
>> 
>> 	PR c++/8171
>> 	* typeck.c (comptypes) <METHOD_TYPE>: Do not fall-through to
>> 	FUNCTION_TYPE, but use a slightly modified version: Compare the
>> 	class-pointers according to the parameter STRICT.
>> 	(build_binary_op) <EQ_EXPR, NE_EXPR>: Allow derived classes when
>> 	comparing ptr-to-member-functions.  Strip pointer before calling
>> 	comptypes.  Add delta offset if necessary.
> 
> How do we get a METHOD_TYPE down into comptypes?  Did we start with a
> pointer-to-member function type, and then pull the METHOD_TYPE out of that?
> 
> Your patch is probably OK, but I'm always unhappy to see METHOD_TYPEs
> moving around the front end because usually these come from
> pointers-to-members, and it's much easier to see that we're following
> the standard if we operate on those types directly by, for example,
> using TYPE_PTRMEM_CLASS_TYPE.  Would you kind walking me through how we
> get here to the comptypes change?  I'm not entirely convinced we even
> need METHOD_TYPEs at all -- but that, of course, is a big change, not to
> be done now.

In order to compare the pointer-to-member-functions we have to compare
the classes to which the member functions belong and the member functions.
The problem is that the first arguments of the functions will differ if
we compare member functions of different classes, while the rest of the
arguments must be identical. I'd be happy to call a function that performs
the checks for me but could find such a thing in the C++ frontend.

Therefore I messed with comptypes to implement the comparison myself:
The METHOD_TYPE case compares the return type, then the first argument
of the functions (allowing derived classes) and finally the remaining
arguments. Do you have a better suggestion?

> Also, I'd be happier with the build_binary_op code if we did it more
> obviously following the standard.

Me, too.

> In particular, call common_type (passing in the two pointer-to-member
> types), convert both arguments to that type, and then compare them
> using the existing code.

Following your suggestion, this seems to do the trick:

  if (!same_type_p (type0, type1))
    {
      type0 = common_type (type0, type1);
      op0 = convert_ptrmem (type0, op0, /*allow_inverse_p=*/true,
                            /*c_cast_p=*/false);
      op1 = convert_ptrmem (type0, op1, /*allow_inverse_p=*/true,
                            /*c_cast_p=*/false);
    }


Btw, here's an updated version of the patch with the simplified code
in build_binary_op.

Regards,
Volker


2007-11-26  Volker Reichelt  <v.reichelt@netcologne.de>

	PR c++/8171
	* typeck.c (structural_comptypes) <METHOD_TYPE>: Do not fall-through
	to FUNCTION_TYPE, but use a slightly modified version: Compare the
	class-pointers according to the parameter STRICT.
	(build_binary_op) <EQ_EXPR, NE_EXPR>: Allow derived classes when
	comparing ptr-to-member-functions.  Strip pointer before calling
	comptypes.  Convert pointers to the common base class, if
	necessary.

===================================================================
--- gcc/gcc/cp/typeck.c	2006-03-14 18:36:29 +0100
+++ gcc/gcc/cp/typeck.c	2006-04-21 01:45:33 +0200
@@ -1028,6 +1028,16 @@ structural_comptypes (tree t1, tree t2, int strict)
       break;
 
     case METHOD_TYPE:
+      if (!same_type_p (TREE_TYPE (t1), TREE_TYPE (t2)))
+	return false;
+      if (!comptypes (TREE_TYPE (TREE_VALUE (TYPE_ARG_TYPES (t1))),
+		      TREE_TYPE (TREE_VALUE (TYPE_ARG_TYPES (t2))), strict))
+	return false;
+      if (!compparms (TREE_CHAIN (TYPE_ARG_TYPES (t1)),
+		      TREE_CHAIN (TYPE_ARG_TYPES (t2))))
+	return false;
+      break;
+
     case FUNCTION_TYPE:
       if (!same_type_p (TREE_TYPE (t1), TREE_TYPE (t2)))
 	return false;
@@ -3393,7 +3403,9 @@ build_binary_op (enum tree_code code,
       else if (TYPE_PTRMEMFUNC_P (type1) && null_ptr_cst_p (op0))
 	return cp_build_binary_op (code, op1, op0);
       else if (TYPE_PTRMEMFUNC_P (type0) && TYPE_PTRMEMFUNC_P (type1)
-	       && same_type_p (type0, type1))
+	       && comptypes (TREE_TYPE (TYPE_PTRMEMFUNC_FN_TYPE (type0)),
+			     TREE_TYPE (TYPE_PTRMEMFUNC_FN_TYPE (type1)),
+			     COMPARE_BASE | COMPARE_DERIVED))
 	{
 	  /* E will be the final comparison.  */
 	  tree e;
@@ -3405,6 +3417,15 @@ build_binary_op (enum tree_code code, tr
 	  tree delta0;
 	  tree delta1;
 
+	  if (!same_type_p (type0, type1))
+	    {
+	      type0 = common_type (type0, type1);
+	      op0 = convert_ptrmem (type0, op0, /*allow_inverse_p=*/true,
+	      			    /*c_cast_p=*/false);
+	      op1 = convert_ptrmem (type0, op1, /*allow_inverse_p=*/true,
+				    /*c_cast_p=*/false);
+	    }
+
 	  if (TREE_SIDE_EFFECTS (op0))
 	    op0 = save_expr (op0);
 	  if (TREE_SIDE_EFFECTS (op1))
===================================================================

2007-11-26  Volker Reichelt  <v.reichelt@netcologne.de>

	PR c++/8171
	* g++.dg/conversion/ptrmemfun1.C: New test.
	* g++.dg/conversion/ptrmemfun2.C: New test.

===================================================================
--- gcc/gcc/testsuite/g++.dg/conversion/ptrmemfun1.C	2005-08-29 00:25:44 +0200
+++ gcc/gcc/testsuite/g++.dg/conversion/ptrmemfun1.C	2006-04-21 17:17:12 +0200
@@ -0,0 +1,43 @@
+// PR c++/8171
+// Origin: <zyzstar@ibl.sk>
+// { dg-do run }
+
+extern "C" void abort();
+
+struct A
+{
+  int i;
+  void foo() {}
+  void bar() {}
+  void baz() {}
+};
+
+struct B
+{
+  int j;
+};
+
+struct C : B, A
+{
+  void baz() {}
+};
+
+int main()
+{
+  void (A::*pa)() = &A::foo;
+  void (C::*pb)() = &C::foo;
+  void (C::*pc)() = &C::bar;
+  void (A::*pd)() = &A::baz;
+  void (C::*pe)() = &C::baz;
+
+  if (pa != pb || pb != pa)
+    abort();
+  if (pa == pc || pc == pa)
+    abort();
+  if (pb == pc || pc == pb)
+    abort();
+  if (pd == pe || pe == pd)
+    abort();
+
+  return 0;
+}
===================================================================
--- gcc/gcc/testsuite/g++.dg/conversion/ptrmemfun2.C	2005-08-29 00:25:44 +0200
+++ gcc/gcc/testsuite/g++.dg/conversion/ptrmemfun2.C	2006-04-21 18:22:20 +0200
@@ -0,0 +1,19 @@
+// PR c++/8171
+// { dg-do compile }
+
+struct A
+{
+  void foo() {}
+};
+
+struct B : A {};
+
+struct C : A {};
+
+int main()
+{
+  void (B::*pb)() = &B::foo;
+  void (C::*pc)() = &C::foo;
+
+  return pb == pc;  // { dg-error "invalid operands" }
+}
===================================================================

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2007-11-30 20:49 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-10-30  9:23 [PATCH] Fix PR c++/8171: Comparing pointer-to-member-functions of derived classes Volker Reichelt
2007-11-12  2:49 ` Mark Mitchell
2007-11-27  0:14   ` Familie Reichelt
2007-11-30 17:00   ` Ollie Wild
2007-11-30 23:03     ` Mark Mitchell
2007-11-27  6:19 Volker Reichelt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).