diff mbox

[C++] PR 51424

Message ID 521F67C1.20104@oracle.com
State New
Headers show

Commit Message

Paolo Carlini Aug. 29, 2013, 3:24 p.m. UTC
Hi,

thus I have this simple patch which at least catches pure 
self-delegation (no cycles). Better than nothing, I would say, given its 
simplicity ;)

At first I thought I would put the check in expand_default_init but then 
I noticed that in case of, eg, virtual bases, the simple pattern 
matching (which looks inside CALL_EXPRs) would miss some cases, like 
when the self-delegation is only in the derived type, not in the base.

Tested x86_64-linux.

Thanks!
Paolo.

/////////////////////////////
/cp
2013-08-29  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/51424
	* cp-tree.h (LOOKUP_DELEGATING_CONS): Add.
	* init.c (perform_target_ctor): Use it.
	* call.c (build_special_member_call): Diagnose self-delegating
	constructors.

/testsuite
2013-08-29  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/51424
	* g++.dg/cpp0x/dc8.C: New.
	* g++.dg/template/meminit1.C: Adjust.

Comments

Jason Merrill Aug. 29, 2013, 7:40 p.m. UTC | #1
On 08/29/2013 11:24 AM, Paolo Carlini wrote:
> +  if ((complain & tf_error)
> +      && (flags & LOOKUP_DELEGATING_CONS)
> +      && name == complete_ctor_identifier
> +      && TREE_CODE (ret) == CALL_EXPR
> +      && (DECL_ABSTRACT_ORIGIN (TREE_OPERAND (CALL_EXPR_FN (ret), 0))
> +	  == current_function_decl))
> +    error ("constructor delegates to itself");

How about doing this check in perform_target_ctor instead, so you don't 
need another LOOKUP flag?

Jason
Paolo Carlini Aug. 30, 2013, 9:06 a.m. UTC | #2
Hi,

On 08/29/2013 09:40 PM, Jason Merrill wrote:
> On 08/29/2013 11:24 AM, Paolo Carlini wrote:
>> +  if ((complain & tf_error)
>> +      && (flags & LOOKUP_DELEGATING_CONS)
>> +      && name == complete_ctor_identifier
>> +      && TREE_CODE (ret) == CALL_EXPR
>> +      && (DECL_ABSTRACT_ORIGIN (TREE_OPERAND (CALL_EXPR_FN (ret), 0))
>> +      == current_function_decl))
>> +    error ("constructor delegates to itself");
> How about doing this check in perform_target_ctor instead, so you 
> don't need another LOOKUP flag?
I should have explained some of that in better detail. The main issue I 
had yesterday, is that the pattern matching can easily become very 
difficult if not impossible: if you look at the second half of 
expand_default_init, in some cases we wrap the returned CALL_EXPR in a 
COND_EXPR, also, even more difficult, in case of constexpr constructors, 
we don't have CALL_EXPRs at all (yesterday my first drafts failed very 
badly for those). Only early enough, around the end of the first 
build_special_member_call call we uniformly have CALL_EXPRs.

I could, for example pass down a separate bit, instead of playing again 
with the LOOKUP_* bits. At some point yesterday I even had that version 
tested ;)

What do you think?

Thanks,
Paolo.
Jason Merrill Aug. 30, 2013, 2:32 p.m. UTC | #3
On 08/30/2013 05:06 AM, Paolo Carlini wrote:
> I should have explained some of that in better detail. The main issue I
> had yesterday, is that the pattern matching can easily become very
> difficult if not impossible: if you look at the second half of
> expand_default_init, in some cases we wrap the returned CALL_EXPR in a
> COND_EXPR

Ah, yes.  Let's go with your first patch, then, rather than make the 
change in multiple places.

Jason
diff mbox

Patch

Index: cp/call.c
===================================================================
--- cp/call.c	(revision 202071)
+++ cp/call.c	(working copy)
@@ -7442,6 +7442,14 @@  build_special_member_call (tree instance, tree nam
   if (allocated != NULL)
     release_tree_vector (allocated);
 
+  if ((complain & tf_error)
+      && (flags & LOOKUP_DELEGATING_CONS)
+      && name == complete_ctor_identifier 
+      && TREE_CODE (ret) == CALL_EXPR
+      && (DECL_ABSTRACT_ORIGIN (TREE_OPERAND (CALL_EXPR_FN (ret), 0))
+	  == current_function_decl))
+    error ("constructor delegates to itself");
+
   return ret;
 }
 
Index: cp/cp-tree.h
===================================================================
--- cp/cp-tree.h	(revision 202071)
+++ cp/cp-tree.h	(working copy)
@@ -4509,6 +4509,8 @@  enum overload_flags { NO_SPECIAL = 0, DTOR_FLAG, T
 #define LOOKUP_NO_RVAL_BIND (LOOKUP_EXPLICIT_TMPL_ARGS << 1)
 /* Used by case_conversion to disregard non-integral conversions.  */
 #define LOOKUP_NO_NON_INTEGRAL (LOOKUP_NO_RVAL_BIND << 1)
+/* Used for delegating constructors in order to diagnose self-delegation.  */
+#define LOOKUP_DELEGATING_CONS (LOOKUP_NO_NON_INTEGRAL << 1)
 
 #define LOOKUP_NAMESPACES_ONLY(F)  \
   (((F) & LOOKUP_PREFER_NAMESPACES) && !((F) & LOOKUP_PREFER_TYPES))
Index: cp/init.c
===================================================================
--- cp/init.c	(revision 202071)
+++ cp/init.c	(working copy)
@@ -500,8 +500,9 @@  perform_target_ctor (tree init)
   tree decl = current_class_ref;
   tree type = current_class_type;
 
-  finish_expr_stmt (build_aggr_init (decl, init, LOOKUP_NORMAL,
-                                     tf_warning_or_error));
+  finish_expr_stmt (build_aggr_init (decl, init,
+				     LOOKUP_NORMAL|LOOKUP_DELEGATING_CONS,
+				     tf_warning_or_error));
   if (TYPE_HAS_NONTRIVIAL_DESTRUCTOR (type))
     {
       tree expr = build_delete (type, decl, sfk_complete_destructor,
Index: testsuite/g++.dg/cpp0x/dc8.C
===================================================================
--- testsuite/g++.dg/cpp0x/dc8.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/dc8.C	(working copy)
@@ -0,0 +1,66 @@ 
+// PR c++/51424
+// { dg-do compile { target c++11 } }
+
+template <class T >
+struct S
+{
+  S() : S() {}          // { dg-error "delegates to itself" }
+  S(int x) : S(x) {}    // { dg-error "delegates to itself" }
+};
+
+struct B1
+{
+  B1() : B1() {}        // { dg-error "delegates to itself" }
+  B1(int y) : B1(y) {}  // { dg-error "delegates to itself" }
+};
+
+struct V1 : virtual B1
+{
+  V1() : B1() {}
+  V1(int x) : B1(x) {}
+};
+
+struct B2
+{
+  B2() : B2() {}        // { dg-error "delegates to itself" }
+  B2(int y) : B2(y) {}  // { dg-error "delegates to itself" }
+};
+
+struct V2 : virtual B2
+{
+  V2() : V2() {}        // { dg-error "delegates to itself" }
+  V2(int x) : V2(x) {}  // { dg-error "delegates to itself" }
+};
+
+struct B3
+{
+  B3() {}
+  B3(int y) {}
+};
+
+struct V3 : virtual B3
+{
+  V3() : V3() {}        // { dg-error "delegates to itself" }
+  V3(int x) : V3(x) {}  // { dg-error "delegates to itself" }
+};
+
+struct CE1
+{
+  constexpr CE1() : CE1() {}        // { dg-error "delegates to itself" }
+  constexpr CE1(int x) : CE1(x) {}  // { dg-error "delegates to itself" }
+};
+
+struct CEB2
+{
+  constexpr CEB2() : CEB2() {}        // { dg-error "delegates to itself" }
+  constexpr CEB2(int x) : CEB2(x) {}  // { dg-error "delegates to itself" }
+};
+
+struct CE2 : CEB2
+{
+  constexpr CE2() : CEB2() {}
+  constexpr CE2(int x) : CEB2(x) {}
+};
+
+S<int> s1;
+S<int> s2(1);
Index: testsuite/g++.dg/template/meminit1.C
===================================================================
--- testsuite/g++.dg/template/meminit1.C	(revision 202068)
+++ testsuite/g++.dg/template/meminit1.C	(working copy)
@@ -3,6 +3,6 @@  template <class T >
 struct S
 {
   S() : S() {} // { dg-message "delegating constructors" }
-};
+}; // { dg-error "delegates to itself" "" { target *-*-* } 5 }
 
 S<int> s;