diff mbox

C++ PATCH for c++/54325 (wrong error initializing abstract base class)

Message ID 50C1790B.3040704@redhat.com
State New
Headers show

Commit Message

Jason Merrill Dec. 7, 2012, 5:05 a.m. UTC 
It's perfectly OK to initialize a base class of abstract type; it's only 
an error to create a full object of such a type.  So this patch moves 
the check from more generic initialization code out into a function 
that's definitely creating a new object.

Tested x86_64-pc-linux-gnu, applying to trunk and 4.7.

Comments

Paolo Carlini Dec. 21, 2012, 11:38 a.m. UTC | #1 
Hi,

On 12/07/2012 06:05 AM, Jason Merrill wrote:
> It's perfectly OK to initialize a base class of abstract type; it's 
> only an error to create a full object of such a type.  So this patch 
> moves the check from more generic initialization code out into a 
> function that's definitely creating a new object.
>
> Tested x86_64-pc-linux-gnu, applying to trunk and 4.7.
I was looking a bit more into this Bug, and something seems still weird 
about the testcase in Comment #1 of the audit trail, which we also 
didn't reject with 4.6.x:

class base
{
     protected:
         base()
         {}
};

class derived : public base
{
     public:
         derived()
             : base{} // <-- Note the c++11 curly brace syntax
         {}
};

int main()
{
     derived d1;
     return 0;
}

???

Thanks,
Paolo.
Jason Merrill Dec. 22, 2012, 8:29 p.m. UTC | #2 
On 12/21/2012 06:38 AM, Paolo Carlini wrote:
> I was looking a bit more into this Bug, and something seems still weird about the testcase in Comment #1 of the audit trail, which we also didn't reject with 4.6.x:

What's weird about it?

Jason
Paolo Carlini Dec. 22, 2012, 11:02 p.m. UTC | #3 
Hi,

Jason Merrill <jason@redhat.com> ha scritto:

>On 12/21/2012 06:38 AM, Paolo Carlini wrote:
>> I was looking a bit more into this Bug, and something seems still
>weird about the testcase in Comment #1 of the audit trail, which we
>also didn't reject with 4.6.x:
>
>What's weird about it?

Well, we still reject it after the patch, whereas we didn't in 4.6.x. Apparently - I didn't check - clang also accepts it. By 'weird' I meant, isn't clear that the Bug is completely resolved...

Paolo
Jason Merrill Dec. 24, 2012, 4:56 a.m. UTC | #4 
On 12/22/2012 06:02 PM, Paolo Carlini wrote:
> Well, we still reject it after the patch

My 4.7 and trunk compilers both accept it (with -std=c++11, of course).

Jason
Paolo Carlini Dec. 24, 2012, 8:29 a.m. UTC | #5 
On 12/24/2012 05:56 AM, Jason Merrill wrote:
> On 12/22/2012 06:02 PM, Paolo Carlini wrote:
>> Well, we still reject it after the patch
>
> My 4.7 and trunk compilers both accept it (with -std=c++11, of course).
I just updated and rebuilt my 4.7 for you (which definitely I didn't 
hack over the next days) and it still rejects it. Likewise mainline. 
Note that the *first* testcase in the Bug Report, the one involving the 
abtract base, now is correctly accepted by both.

Are you sure your patch handles the access control issue too?? (isn't 
obvious to me that it does, looking at the patch itself and your comments)

Thanks,
Paolo.
Jason Merrill Dec. 26, 2012, 6:58 p.m. UTC | #6 
On 12/24/2012 03:29 AM, Paolo Carlini wrote:
> Are you sure your patch handles the access control issue too?? (isn't
> obvious to me that it does, looking at the patch itself and your comments)

Nope, you're right.  I put the testcase in one file and then compiled a 
different one.  /facepalm

Jason
diff mbox

Patch

commit 6fb305c7c88b07c429e8a39fbd514a417c5b6127
Author: jason <jason@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Fri Dec 7 04:54:27 2012 +0000

    	PR c++/54325
    	* tree.c (build_aggr_init_expr): Don't check for abstract class.
    
diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index 28ff0f2..ca82f75 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -407,18 +407,13 @@  build_aggr_init_array (tree return_type, tree fn, tree slot, int nargs,
    callable.  */
 
 tree
-build_aggr_init_expr (tree type, tree init, tsubst_flags_t complain)
+build_aggr_init_expr (tree type, tree init, tsubst_flags_t /*complain*/)
 {
   tree fn;
   tree slot;
   tree rval;
   int is_ctor;
 
-  /* Make sure that we're not trying to create an instance of an
-     abstract class.  */
-  if (abstract_virtuals_error_sfinae (NULL_TREE, type, complain))
-    return error_mark_node;
-
   if (TREE_CODE (init) == CALL_EXPR)
     fn = CALL_EXPR_FN (init);
   else if (TREE_CODE (init) == AGGR_INIT_EXPR)
@@ -477,6 +472,11 @@  build_cplus_new (tree type, tree init, tsubst_flags_t complain)
   tree rval = build_aggr_init_expr (type, init, complain);
   tree slot;
 
+  /* Make sure that we're not trying to create an instance of an
+     abstract class.  */
+  if (abstract_virtuals_error_sfinae (NULL_TREE, type, complain))
+    return error_mark_node;
+
   if (TREE_CODE (rval) == AGGR_INIT_EXPR)
     slot = AGGR_INIT_EXPR_SLOT (rval);
   else if (TREE_CODE (rval) == CALL_EXPR
diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist-pure.C b/gcc/testsuite/g++.dg/cpp0x/initlist-pure.C
new file mode 100644
index 0000000..63c341c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/initlist-pure.C
@@ -0,0 +1,25 @@ 
+// PR c++/54325
+// { dg-options -std=c++11 }
+
+class Base {
+public:
+  Base() {};
+  virtual ~Base() {};
+
+  virtual void do_stuff() = 0;
+};
+
+class Derived: public Base {
+public:
+  Derived() : Base{} {};
+  virtual ~Derived() {};
+
+  virtual void do_stuff() {};
+};
+
+int
+main() {
+  Derived d;
+
+  return 0;
+}
diff --git a/gcc/testsuite/g++.dg/other/abstract3.C b/gcc/testsuite/g++.dg/other/abstract3.C
index 528b7d7..95e293e 100644
--- a/gcc/testsuite/g++.dg/other/abstract3.C
+++ b/gcc/testsuite/g++.dg/other/abstract3.C
@@ -8,5 +8,5 @@  struct A                  // { dg-message "note" }
 struct B
 {
   A a;           // { dg-error "abstract" }
-  B() : a() {}   // { dg-error "abstract" }
+  B() : a() {}
 };