Re: [Patch] PR c++/26256
On 10/10/2011 03:59 PM, Fabien Chêne wrote: Sorry but I've failed to see why you called them callers of lookup_field_1, could you elaborate ? Hmm, I was assuming that the other functions would have gotten their decls via lookup_field_1, but I suppose that isn't true for unqualified lookup that finds the name in class_binding_level. Never mind. Jason
Re: [Patch] PR c++/26256
Hi, 2011/9/26 Jason Merrill : > On 09/25/2011 05:06 PM, Fabien Chêne wrote: >> >> + else if ((using_decl = strip_using_decl (member)) != member) > >> + /* If it is a using decl, use its underlying decl. */ >> + type_decl = strip_using_decl (type_decl); > >> - if (DECL_NAME (field) == name >> + if (DECL_NAME (decl) == name >> && (!want_type >> - || TREE_CODE (field) == TYPE_DECL >> - || DECL_CLASS_TEMPLATE_P (field))) >> - return field; >> + || TREE_CODE (decl) == TYPE_DECL >> + || DECL_CLASS_TEMPLATE_P (decl))) >> + return decl; > > Why do we need to strip the USING_DECL both in lookup_field_1 and in > callers? Sorry but I've failed to see why you called them callers of lookup_field_1, could you elaborate ? -- Fabien
Re: [Patch] PR c++/26256
On 09/25/2011 05:06 PM, Fabien Chêne wrote: + else if ((using_decl = strip_using_decl (member)) != member) + /* If it is a using decl, use its underlying decl. */ + type_decl = strip_using_decl (type_decl); - if (DECL_NAME (field) == name + if (DECL_NAME (decl) == name && (!want_type - || TREE_CODE (field) == TYPE_DECL - || DECL_CLASS_TEMPLATE_P (field))) - return field; + || TREE_CODE (decl) == TYPE_DECL + || DECL_CLASS_TEMPLATE_P (decl))) + return decl; Why do we need to strip the USING_DECL both in lookup_field_1 and in callers? + /* do not push the decl more than once */ Comments should start with a capital letter and end with a period. This should also say "unless we need to compare underlying types at instantiation time" + /* We allow pushing an enum multiple times in a class + * template in order to handle late matching of underlying + * type on an opaque-enum-declaration followed by an + * enum-specifier. */ And we don't put * at the beginning of each line. + enumtype = xref_tag (enum_type, name, /*tag_scope=*/ts_current, false); + /* TARGET_BVAL is error_mark_node when TARGET_DECL's name has been used + else if (TREE_CODE (target_bval) == TYPE_DECL && DECL_ARTIFICIAL (target_bval) + || same_type_p (TREE_TYPE (target_decl), TREE_TYPE (target_bval + else if (TREE_CODE (target_decl) == VAR_DECL && TREE_CODE (target_bval) == VAR_DECL + if (TREE_CODE (target_decl) == TYPE_DECL && DECL_ARTIFICIAL (target_decl)) + else if (TREE_CODE (target_decl) == OVERLOAD && is_overloaded_fn (target_bval)) + else if (TREE_CODE (decl) == USING_DECL && is_overloaded_fn (target_bval)) + else if (TREE_CODE (bval) == USING_DECL && is_overloaded_fn (target_decl)) These lines go past 80 characters. Jason
Re: [Patch] PR c++/26256
Jason,
Please ignore the previous patch, where I have introduced an
unintentional modification for PR c++/30195 in search.c, here is a new
one, sorry about that.
--
Fabien
Index: gcc/testsuite/g++.old-deja/g++.other/using1.C
===
--- gcc/testsuite/g++.old-deja/g++.other/using1.C (revision 178088)
+++ gcc/testsuite/g++.old-deja/g++.other/using1.C (working copy)
@@ -16,12 +16,12 @@ public:
using B::b;
};
-class D2 : public B { // { dg-error "" } conflicting access specifications
+class D2 : public B {
public:
using B::a;
- using B::b;
+ using B::b; // { dg-message "" } conflicting declaration
private:
- using B::b;
+ using B::b; // { dg-error "" } conflicts
};
Index: gcc/testsuite/g++.dg/debug/using4.C
===
--- gcc/testsuite/g++.dg/debug/using4.C (revision 0)
+++ gcc/testsuite/g++.dg/debug/using4.C (revision 0)
@@ -0,0 +1,24 @@
+// PR c++/26256
+// { dg-do compile }
+
+struct A
+{
+typedef char type;
+};
+
+struct B
+{
+typedef int type;
+};
+
+struct C : A, B
+{
+using A::type;
+type f (type);
+};
+
+C::type C::f( type )
+{
+type c = 'e';
+return c;
+}
Index: gcc/testsuite/g++.dg/debug/using5.C
===
--- gcc/testsuite/g++.dg/debug/using5.C (revision 0)
+++ gcc/testsuite/g++.dg/debug/using5.C (revision 0)
@@ -0,0 +1,23 @@
+// PR c++/26256
+// { dg-do compile }
+
+struct A
+{
+int i;
+};
+
+struct B
+{
+int i;
+};
+
+struct C : A, B
+{
+using B::i;
+int f ();
+};
+
+int C::f()
+{
+return i;
+}
Index: gcc/testsuite/g++.dg/lookup/using36.C
===
--- gcc/testsuite/g++.dg/lookup/using36.C (revision 0)
+++ gcc/testsuite/g++.dg/lookup/using36.C (revision 0)
@@ -0,0 +1,31 @@
+// PR c++/25994
+// { dg-do run }
+
+struct B1
+{
+ void f (char) {}
+ void f (double) { __builtin_abort(); }
+};
+
+struct B2
+{
+ void f (double) { __builtin_abort(); }
+ void f (int) {}
+};
+
+struct D : public B1, public B2
+{
+ using B1::f;
+ using B2::f;
+ void g ()
+ {
+f ('a'); // should call B1::f(char)
+f (33);// should call B2::f(int)
+ }
+};
+
+int main()
+{
+ D d;
+ d.g();
+}
Index: gcc/testsuite/g++.dg/lookup/using24.C
===
--- gcc/testsuite/g++.dg/lookup/using24.C (revision 0)
+++ gcc/testsuite/g++.dg/lookup/using24.C (revision 0)
@@ -0,0 +1,12 @@
+// PR c++/26256
+// { dg-do compile }
+
+struct A { int next; };
+struct B { int next; };
+struct C : B { using B::next; };
+
+struct D : A, C
+{
+ using C::next;
+ void f() { next = 1; }
+};
Index: gcc/testsuite/g++.dg/lookup/using28.C
===
--- gcc/testsuite/g++.dg/lookup/using28.C (revision 0)
+++ gcc/testsuite/g++.dg/lookup/using28.C (revision 0)
@@ -0,0 +1,11 @@
+// PR c++/26256
+// { dg-do compile }
+
+struct A { int f; };
+struct B { int f; };
+struct C : A, B { using B::f; };
+
+struct D : C
+{
+void g() { f = 1; }
+};
Index: gcc/testsuite/g++.dg/lookup/using33.C
===
--- gcc/testsuite/g++.dg/lookup/using33.C (revision 0)
+++ gcc/testsuite/g++.dg/lookup/using33.C (revision 0)
@@ -0,0 +1,26 @@
+// { dg-do run }
+
+template
+struct Foo
+{
+ int k (float) {return 0;}
+};
+
+template
+struct Baz
+{
+ int k (int) {return 1;}
+};
+
+template
+struct Bar : Foo , Baz
+{
+ using Foo::k;
+ using Baz::k;
+};
+
+int main()
+{
+ Bar bar;
+ return bar.k( 1.0f );
+}
Index: gcc/testsuite/g++.dg/lookup/using25.C
===
--- gcc/testsuite/g++.dg/lookup/using25.C (revision 0)
+++ gcc/testsuite/g++.dg/lookup/using25.C (revision 0)
@@ -0,0 +1,28 @@
+// PR c++/26256
+// { dg-do run }
+
+struct A
+{
+int next;
+};
+
+struct B
+{
+int next;
+};
+
+struct C : public A, public B
+{
+using A::next;
+};
+
+void foo(C& c) { c.next = 42; }
+
+int main()
+{
+C c;
+foo (c);
+c.B::next = 12;
+if (c.next != 42 || c.A::next != 42 || c.B::next != 12)
+ __builtin_abort();
+}
Index: gcc/testsuite/g++.dg/lookup/using29.C
===
--- gcc/testsuite/g++.dg/lookup/using29.C (revision 0)
+++ gcc/testsuite/g++.dg/lookup/using29.C (revision 0)
@@ -0,0 +1,81 @@
+// { dg-do compile }
+
+struct A
+{
+ int i;
+};
+
+struct B
+{
+ int i;
+};
+
+struct C : A, B
+{
+ using A::i; // { dg-error "conflicts with previous" }
+ using B::i; // { dg-error "declaration" }
+};
+
+struct E
+{
+ typedef int type;
+};
+
+struct F
+{
+ typedef int type;
+};
+
+struct G : E, F
+{
+ using E::type; // { dg-error "conflicts with previous" }
+
Re: [Patch] PR c++/26256
Hi, > Indeed, I've removed the blank trailing lines, and some in the middle, > not all though, I like it readable as well ;-) Thanks! Paolo
Re: [Patch] PR c++/26256
2011/9/25 Paolo Carlini :
> ... nitpicking, of course, but in the testcases you have many blank trailing
> lines (and also some gratuitus, imho, blank lines in the middle)
Indeed, I've removed the blank trailing lines, and some in the middle,
not all though, I like it readable as well ;-)
--
Fabien
Index: gcc/testsuite/g++.old-deja/g++.other/using1.C
===
--- gcc/testsuite/g++.old-deja/g++.other/using1.C (revision 178088)
+++ gcc/testsuite/g++.old-deja/g++.other/using1.C (working copy)
@@ -16,12 +16,12 @@ public:
using B::b;
};
-class D2 : public B { // { dg-error "" } conflicting access specifications
+class D2 : public B {
public:
using B::a;
- using B::b;
+ using B::b; // { dg-message "" } conflicting declaration
private:
- using B::b;
+ using B::b; // { dg-error "" } conflicts
};
Index: gcc/testsuite/g++.dg/debug/using4.C
===
--- gcc/testsuite/g++.dg/debug/using4.C (revision 0)
+++ gcc/testsuite/g++.dg/debug/using4.C (revision 0)
@@ -0,0 +1,24 @@
+// PR c++/26256
+// { dg-do compile }
+
+struct A
+{
+typedef char type;
+};
+
+struct B
+{
+typedef int type;
+};
+
+struct C : A, B
+{
+using A::type;
+type f (type);
+};
+
+C::type C::f( type )
+{
+type c = 'e';
+return c;
+}
Index: gcc/testsuite/g++.dg/debug/using5.C
===
--- gcc/testsuite/g++.dg/debug/using5.C (revision 0)
+++ gcc/testsuite/g++.dg/debug/using5.C (revision 0)
@@ -0,0 +1,23 @@
+// PR c++/26256
+// { dg-do compile }
+
+struct A
+{
+int i;
+};
+
+struct B
+{
+int i;
+};
+
+struct C : A, B
+{
+using B::i;
+int f ();
+};
+
+int C::f()
+{
+return i;
+}
Index: gcc/testsuite/g++.dg/lookup/using36.C
===
--- gcc/testsuite/g++.dg/lookup/using36.C (revision 0)
+++ gcc/testsuite/g++.dg/lookup/using36.C (revision 0)
@@ -0,0 +1,31 @@
+// PR c++/25994
+// { dg-do run }
+
+struct B1
+{
+ void f (char) {}
+ void f (double) { __builtin_abort(); }
+};
+
+struct B2
+{
+ void f (double) { __builtin_abort(); }
+ void f (int) {}
+};
+
+struct D : public B1, public B2
+{
+ using B1::f;
+ using B2::f;
+ void g ()
+ {
+f ('a'); // should call B1::f(char)
+f (33);// should call B2::f(int)
+ }
+};
+
+int main()
+{
+ D d;
+ d.g();
+}
Index: gcc/testsuite/g++.dg/lookup/using24.C
===
--- gcc/testsuite/g++.dg/lookup/using24.C (revision 0)
+++ gcc/testsuite/g++.dg/lookup/using24.C (revision 0)
@@ -0,0 +1,12 @@
+// PR c++/26256
+// { dg-do compile }
+
+struct A { int next; };
+struct B { int next; };
+struct C : B { using B::next; };
+
+struct D : A, C
+{
+ using C::next;
+ void f() { next = 1; }
+};
Index: gcc/testsuite/g++.dg/lookup/using28.C
===
--- gcc/testsuite/g++.dg/lookup/using28.C (revision 0)
+++ gcc/testsuite/g++.dg/lookup/using28.C (revision 0)
@@ -0,0 +1,11 @@
+// PR c++/26256
+// { dg-do compile }
+
+struct A { int f; };
+struct B { int f; };
+struct C : A, B { using B::f; };
+
+struct D : C
+{
+void g() { f = 1; }
+};
Index: gcc/testsuite/g++.dg/lookup/using33.C
===
--- gcc/testsuite/g++.dg/lookup/using33.C (revision 0)
+++ gcc/testsuite/g++.dg/lookup/using33.C (revision 0)
@@ -0,0 +1,26 @@
+// { dg-do run }
+
+template
+struct Foo
+{
+ int k (float) {return 0;}
+};
+
+template
+struct Baz
+{
+ int k (int) {return 1;}
+};
+
+template
+struct Bar : Foo , Baz
+{
+ using Foo::k;
+ using Baz::k;
+};
+
+int main()
+{
+ Bar bar;
+ return bar.k( 1.0f );
+}
Index: gcc/testsuite/g++.dg/lookup/using25.C
===
--- gcc/testsuite/g++.dg/lookup/using25.C (revision 0)
+++ gcc/testsuite/g++.dg/lookup/using25.C (revision 0)
@@ -0,0 +1,28 @@
+// PR c++/26256
+// { dg-do run }
+
+struct A
+{
+int next;
+};
+
+struct B
+{
+int next;
+};
+
+struct C : public A, public B
+{
+using A::next;
+};
+
+void foo(C& c) { c.next = 42; }
+
+int main()
+{
+C c;
+foo (c);
+c.B::next = 12;
+if (c.next != 42 || c.A::next != 42 || c.B::next != 12)
+ __builtin_abort();
+}
Index: gcc/testsuite/g++.dg/lookup/using29.C
===
--- gcc/testsuite/g++.dg/lookup/using29.C (revision 0)
+++ gcc/testsuite/g++.dg/lookup/using29.C (revision 0)
@@ -0,0 +1,81 @@
+// { dg-do compile }
+
+struct A
+{
+ int i;
+};
+
+struct B
+{
+ int i;
+};
+
+struct C : A, B
+{
+ using A::i; // { dg-error "conflicts with previous" }
+ using B::i; // { dg-error "declaration" }
+};
+
+struct E
+{
+ typedef int type
Re: [Patch] PR c++/26256
... nitpicking, of course, but in the testcases you have many blank trailing lines (and also some gratuitus, imho, blank lines in the middle) Paolo
Re: [Patch] PR c++/26256
I've updated the patch. I can't resist to tackle PR 25994 at the same time. There was a diagnostic about conflicting using declarations in add_method, which is no longer necessary and bogus in the case of PR 25994, so I just removed it. Duplicated using declarations are now diagnosed in supplement_binding_1. Tested x86_64-unknown-linux-gnu, OK to commit? gcc/ChangeLog 2011-09-21 Fabien Chêne PR c++/26256 * dbxout.c (dbxout_type_fields): Ignore using declarations. gcc/testsuite/ChangeLog 2011-09-21 Fabien Chêne PR c++/26256 PR c++/25994 * g++.dg/lookup/using23.C: New. * g++.dg/lookup/using24.C: New. * g++.dg/lookup/using25.C: New. * g++.dg/lookup/using26.C: New. * g++.dg/lookup/using27.C: New. * g++.dg/lookup/using28.C: New. * g++.dg/lookup/using29.C: New. * g++.dg/lookup/using30.C: New. * g++.dg/lookup/using31.C: New. * g++.dg/lookup/using32.C: New. * g++.dg/lookup/using33.C: New. * g++.dg/lookup/using34.C: New. * g++.dg/lookup/using35.C: New. * g++.dg/lookup/using36.C: New. * g++.dg/debug/using4.C: New. * g++.dg/debug/using5.C: New. * g++.dg/cpp0x/forw_enum10.C: New. * g++.old-deja/g++.other/using1.C: Adjust. * g++.dg/template/using2.C: Likewise. gcc/cp/ChangeLog 2011-09-21 Fabien Chêne PR c++/26256 PR c++/25994 * search.c (lookup_field_1): Get rid of the comment saying that USING_DECL should not be returned, and actually return USING_DECL if appropriate. * semantics.c (finish_member_declaration): Remove the check that prevents USING_DECLs from being verified by pushdecl_class_level. * typeck.c (build_class_member_access_expr): Handle USING_DECLs. * class.c (check_field_decls): Keep using declarations. (add_method): Remove a wrong diagnostic about conflicting using declarations. * parser.c (cp_parser_nonclass_name): Handle USING_DECLs. * decl.c (start_enum): Call xref_tag whenever possible. * name-lookup.c (strip_using_decl): New function. (supplement_binding_1): Call strip_using_decl on decl and bval. Perform most of the checks with USING_DECLs stripped. Also check that the target decl and the target bval does not refer to the same declaration. Allow pushing an enum multiple times in a template class. (push_class_level_binding): Call strip_using_decl on decl and bval. Perform most of the checks with USING_DECLs stripped. Return true if both decl and bval refer to USING_DECLs and are dependent. -- Fabien patch_26256 Description: Binary data
Re: [Patch] PR c++/26256
2011/9/23 Jason Merrill :
> On 09/22/2011 05:11 PM, Fabien Chêne wrote:
>>
>> 2011/9/22 Jason Merrill:
>
>>> I don't, it just seemed strange to handle functions differently from
>>> other
>>> decls here. But when I look more closely I see that we're in
>>> lookup_field_1, which isn't interested in functions, so I guess we do
>>> want
>>> to ignore function using-declarations here.
>>
>> That's strange because if we do return FUNCTION_DECL, PR c++/30195 seems
>> solved.
>
> It works for that testcase, but we need to handle functions in
> lookup_fnfields_1 since it's also called from other places.
Aha, hence, I'll tackle this issue in another patch, one PR at a time !
>>> But check for is_overloaded_fn rather than just OVERLOAD. Also, it looks
>>> like the new code doesn't respect want_type.
>>
>> Er, I'm a bit lost, do you mean something like that ?
>>
>> if (TREE_CODE (field) == USING_DECL)
>> {
>> tree target_field = strip_using_decl (field);
>> if (target_field != field)
>> {
>> if (DECL_P (target_field)&& DECL_NAME (target_field) == name
>> || (is_overloaded_fn (target_field)
>> && DECL_NAME (get_first_fn (target_field)) == name))
>> {
>> if (!want_type
>> || TREE_CODE (target_field) == TYPE_DECL)
>> return target_field;
>> }
>>
>> continue;
>> }
>> }
>
> I was thinking more like
>
> tree decl = field;
> if (TREE_CODE (decl) == USING_DECL)
> {
> decl = strip_using_decl (decl);
> if (is_overloaded_fn (decl)) continue;
> }
> if (DECL_NAME (decl) == name
> ...
I should have got it... Thank you anyway.
I will update the patch accordingly at the begining of the next week, I hope.
--
Fabien
Re: [Patch] PR c++/26256
On 09/22/2011 05:11 PM, Fabien Chêne wrote:
2011/9/22 Jason Merrill:
I don't, it just seemed strange to handle functions differently from other
decls here. But when I look more closely I see that we're in
lookup_field_1, which isn't interested in functions, so I guess we do want
to ignore function using-declarations here.
That's strange because if we do return FUNCTION_DECL, PR c++/30195 seems solved.
It works for that testcase, but we need to handle functions in
lookup_fnfields_1 since it's also called from other places.
But check for is_overloaded_fn rather than just OVERLOAD. Also, it looks like
the new code doesn't respect want_type.
Er, I'm a bit lost, do you mean something like that ?
if (TREE_CODE (field) == USING_DECL)
{
tree target_field = strip_using_decl (field);
if (target_field != field)
{
if (DECL_P (target_field)&& DECL_NAME (target_field) == name
|| (is_overloaded_fn (target_field)
&& DECL_NAME (get_first_fn (target_field)) == name))
{
if (!want_type
|| TREE_CODE (target_field) == TYPE_DECL)
return target_field;
}
continue;
}
}
I was thinking more like
tree decl = field;
if (TREE_CODE (decl) == USING_DECL)
{
decl = strip_using_decl (decl);
if (is_overloaded_fn (decl)) continue;
}
if (DECL_NAME (decl) == name
...
Jason
Re: [Patch] PR c++/26256
2011/9/22 Jason Merrill :
> On 09/22/2011 04:22 AM, Fabien Chêne wrote:
>>
>> I would have thought that we want to do something with OVERLOAD here,
>> in order to get rid of PR c++/30195 and c++/25994 (removing a wrong
>> diagnostic additionaly)... But those PRs are already fixed by this
>> patch without doing anything with OVERLOAD. Consequently, I don't
>> really know why it would be needed, but I can certainly do it if you
>> prefer. Have you got an example in mind where it would be needed ?
>
> I don't, it just seemed strange to handle functions differently from other
> decls here. But when I look more closely I see that we're in
> lookup_field_1, which isn't interested in functions, so I guess we do want
> to ignore function using-declarations here.
That's strange because if we do return FUNCTION_DECL, PR c++/30195 seems solved.
> But check for is_overloaded_fn rather than just OVERLOAD. Also, it looks
> like the new code doesn't respect want_type.
Er, I'm a bit lost, do you mean something like that ?
if (TREE_CODE (field) == USING_DECL)
{
tree target_field = strip_using_decl (field);
if (target_field != field)
{
if (DECL_P (target_field) && DECL_NAME (target_field) == name
|| (is_overloaded_fn (target_field)
&& DECL_NAME (get_first_fn (target_field)) == name))
{
if (!want_type
|| TREE_CODE (target_field) == TYPE_DECL)
return target_field;
}
continue;
}
}
Thanks,
--
Fabien
Re: [Patch] PR c++/26256
On 09/22/2011 04:22 AM, Fabien Chêne wrote: I would have thought that we want to do something with OVERLOAD here, in order to get rid of PR c++/30195 and c++/25994 (removing a wrong diagnostic additionaly)... But those PRs are already fixed by this patch without doing anything with OVERLOAD. Consequently, I don't really know why it would be needed, but I can certainly do it if you prefer. Have you got an example in mind where it would be needed ? I don't, it just seemed strange to handle functions differently from other decls here. But when I look more closely I see that we're in lookup_field_1, which isn't interested in functions, so I guess we do want to ignore function using-declarations here. But check for is_overloaded_fn rather than just OVERLOAD. Also, it looks like the new code doesn't respect want_type. Jason
Re: [Patch] PR c++/26256
2011/9/21 Jason Merrill :
> On 09/21/2011 01:59 PM, Fabien Chêne wrote:
if (!DECL_DEPENDENT_P (field))
- continue;
+ {
+ tree using_decl = USING_DECL_DECLS (field);
+ if ((TREE_CODE (using_decl) == FIELD_DECL
+ || TREE_CODE (using_decl) == TYPE_DECL)
+&& DECL_NAME (using_decl) == name)
+ return using_decl;
+ continue;
+ }
>>>
>>> This section needs a comment. Why do we look through USING_DECL for
>>> these
>>> two kinds of member but not others?
>>
>> I was looking explicitely for a FIELD_DECL or a TYPE_DECL because it
>> was crashing if I didn't. In fact, it was simply that DECL_NAME needs
>> at least tree_minimal, which OVERLOAD doesn't have. Is there a way to
>> properly check that DECL_NAME will succeed ?
>
> You could check DECL_P first, but don't we want to return the OVERLOAD here,
> too? You can use get_first_fn to get a FUNCTION_DECL out of anything that
> satisfies is_overloaded_fn.
I would have thought that we want to do something with OVERLOAD here,
in order to get rid of PR c++/30195 and c++/25994 (removing a wrong
diagnostic additionaly)... But those PRs are already fixed by this
patch without doing anything with OVERLOAD. Consequently, I don't
really know why it would be needed, but I can certainly do it if you
prefer. Have you got an example in mind where it would be needed ?
--
Fabien
Re: [Patch] PR c++/26256
On 09/21/2011 01:59 PM, Fabien Chêne wrote:
if (!DECL_DEPENDENT_P (field))
- continue;
+ {
+ tree using_decl = USING_DECL_DECLS (field);
+ if ((TREE_CODE (using_decl) == FIELD_DECL
+ || TREE_CODE (using_decl) == TYPE_DECL)
+&& DECL_NAME (using_decl) == name)
+ return using_decl;
+ continue;
+ }
This section needs a comment. Why do we look through USING_DECL for these
two kinds of member but not others?
I was looking explicitely for a FIELD_DECL or a TYPE_DECL because it
was crashing if I didn't. In fact, it was simply that DECL_NAME needs
at least tree_minimal, which OVERLOAD doesn't have. Is there a way to
properly check that DECL_NAME will succeed ?
You could check DECL_P first, but don't we want to return the OVERLOAD
here, too? You can use get_first_fn to get a FUNCTION_DECL out of
anything that satisfies is_overloaded_fn.
Jason
Re: [Patch] PR c++/26256
... with the ChangeLog gcc/ChangeLog 2011-09-21 Fabien Chêne PR c++/26256 * dbxout.c (dbxout_type_fields): Ignore using declarations. gcc/testsuite/ChangeLog 2011-09-21 Fabien Chêne PR c++/26256 * g++.dg/lookup/using23.C: New. * g++.dg/lookup/using24.C: New. * g++.dg/lookup/using25.C: New. * g++.dg/lookup/using26.C: New. * g++.dg/lookup/using27.C: New. * g++.dg/lookup/using28.C: New. * g++.dg/lookup/using29.C: New. * g++.dg/lookup/using30.C: New. * g++.dg/lookup/using31.C: New. * g++.dg/lookup/using32.C: New. * g++.dg/lookup/using33.C: New. * g++.dg/lookup/using34.C: New. * g++.dg/lookup/using35.C: New. * g++.dg/debug/using4.C: New. * g++.dg/debug/using5.C: New. * g++.dg/cpp0x/forw_enum10.C: New. * g++.old-deja/g++.other/using1.C: Adjust. * g++.dg/template/using2.C: Likewise. gcc/cp/ChangeLog 2011-09-21 Fabien Chêne PR c++/26256 * search.c (lookup_field_1): Get rid of the comment saying that USING_DECL should not be returned, and actually return USING_DECL if appropriate. * semantics.c (finish_member_declaration): Remove the check that prevents USING_DECLs from being verified by pushdecl_class_level. * typeck.c (build_class_member_access_expr): Handle USING_DECLs. * class.c (check_field_decls): Keep using declarations. * parser.c (cp_parser_nonclass_name): Handle USING_DECLs. * decl.c (start_enum): Call xref_tag whenever possible. * name-lookup.c (strip_using_decl): New function. (supplement_binding_1): Call strip_using_decl on decl and bval. Perform most of the checks with USING_DECLs stripped. Also check that the target decl and the target bval does not refer to the same declaration. Allow pushing an enum multiple times in a template class. (push_class_level_binding): Call strip_using_decl on decl and bval. Perform most of the checks with USING_DECLs stripped. Return true if both decl and bval refer to USING_DECLs and are dependent. -- Fabien
Re: [Patch] PR c++/26256
Hi,
2011/9/20 Jason Merrill :
> On 09/17/2011 09:44 AM, Fabien Chêne wrote:
>>
>> I tried various things without success, and I ended up hacking
>> supplement_binding_1 to handle those ENUMERAL_TYPEs.
>> I am all ear for another solution ...
>
> Your solution seems reasonable to me, but it needs a comment, along the
> lines of
>
> /* We allow pushing an enum multiple times in a class template in order to
> handle late matching of underlying type on an opaque-enum-declaration
> followed by an enum-specifier. */
Done.
> And I guess limit it to dependent class scope. Incidentally, repeating
> opaque-enum-declarations at class scope is invalid under 9.2/1:
>
> --
> A member shall not be declared twice in the member-specification, except
> that a nested class or member class template can be declared and then later
> defined, and except that an enumeration can be introduced with an
> opaque-enum-declaration and later redeclared with an enum-specifier.
> --
>
> So
>
> struct A
> {
> enum E: int;
> enum E: int { e1 };
> };
>
> is OK, but
>
> struct B
> {
> enum E: int;
> enum E: int;
> };
>
> is not.
Hence, I think there is currently a bug here. Moreover, some tests are
checking the above example (forw_enum{3,4}.C at least). However, I
think that fixing this bug is beyond the scope of this patch. I'll
fill in a bug for this issue.
>> +static tree
>> +strip_using_decl (tree decl)
>
> Needs a comment. Also, this function has a loop in it, but various other
> places in the patch that look through USING_DECLs don't loop.
Done.
>> if (!DECL_DEPENDENT_P (field))
>> - continue;
>> + {
>> + tree using_decl = USING_DECL_DECLS (field);
>> + if ((TREE_CODE (using_decl) == FIELD_DECL
>> + || TREE_CODE (using_decl) == TYPE_DECL)
>> + && DECL_NAME (using_decl) == name)
>> + return using_decl;
>> + continue;
>> + }
>
> This section needs a comment. Why do we look through USING_DECL for these
> two kinds of member but not others?
I was looking explicitely for a FIELD_DECL or a TYPE_DECL because it
was crashing if I didn't. In fact, it was simply that DECL_NAME needs
at least tree_minimal, which OVERLOAD doesn't have. Is there a way to
properly check that DECL_NAME will succeed ?
Attached an updated patch, regtested on x86_64-unknown-linux-gnu.
--
Fabien
pr26256.patch
Description: Binary data
Re: [Patch] PR c++/26256
On 09/17/2011 09:44 AM, Fabien Chêne wrote:
I tried various things without success, and I ended up hacking
supplement_binding_1 to handle those ENUMERAL_TYPEs.
I am all ear for another solution ...
Your solution seems reasonable to me, but it needs a comment, along the
lines of
/* We allow pushing an enum multiple times in a class template in order
to handle late matching of underlying type on an opaque-enum-declaration
followed by an enum-specifier. */
And I guess limit it to dependent class scope. Incidentally, repeating
opaque-enum-declarations at class scope is invalid under 9.2/1:
--
A member shall not be declared twice in the member-specification, except
that a nested class or member class template can be declared and then
later defined, and except that an enumeration can be introduced with an
opaque-enum-declaration and later redeclared with an enum-specifier.
--
So
struct A
{
enum E: int;
enum E: int { e1 };
};
is OK, but
struct B
{
enum E: int;
enum E: int;
};
is not.
+static tree
+strip_using_decl (tree decl)
Needs a comment. Also, this function has a loop in it, but various
other places in the patch that look through USING_DECLs don't loop.
if (!DECL_DEPENDENT_P (field))
- continue;
+ {
+ tree using_decl = USING_DECL_DECLS (field);
+ if ((TREE_CODE (using_decl) == FIELD_DECL
+ || TREE_CODE (using_decl) == TYPE_DECL)
+ && DECL_NAME (using_decl) == name)
+ return using_decl;
+ continue;
+ }
This section needs a comment. Why do we look through USING_DECL for
these two kinds of member but not others?
Re: [Patch] PR c++/26256
Hi!
(Again, sorry for the long delay, but I indeed have very few time to work on it)
2011/6/22 Jason Merrill :
> On 06/15/2011 01:58 PM, Fabien Chęne wrote:
>>
>> Otherwise, perhaps that it would be better if the second declaration
>> of E1 does not rely on supplement_binding_1.
>> What do you think ?
>
> I agree. We should be using xref_tag for this like we do with classes, so
> we don't try to push the same tag more than once.
Ah, yes it works for the previous testcase, but I can't manage to make
it work for the below example:
template struct S1
{
enum E : T; // { dg-error "previous definition" }
enum E : int; // { dg-error "different underlying type" }
};
template struct S1; // { dg-message "required from here" }
I tried various things without success, and I ended up hacking
supplement_binding_1 to handle those ENUMERAL_TYPEs.
I am all ear for another solution ...
Attached is an updated patch that should address most of your previous
comments (except for c++0X enums). Tested x86_64_unknown-linux-gnu
without new regressions.
Is it in better shape ?
gcc/ChangeLog
2011-09-15 Fabien Chêne
PR c++/26256
* dbxout.c (dbxout_type_fields): Ignore using declarations.
gcc/testsuite/ChangeLog
2011-09-15 Fabien Chêne
PR c++/26256
* g++.dg/lookup/using23.C: New.
* g++.dg/lookup/using24.C: New.
* g++.dg/lookup/using25.C: New.
* g++.dg/lookup/using26.C: New.
* g++.dg/lookup/using27.C: New.
* g++.dg/lookup/using28.C: New.
* g++.dg/lookup/using29.C: New.
* g++.dg/lookup/using30.C: New.
* g++.dg/lookup/using31.C: New.
* g++.dg/lookup/using32.C: New.
* g++.dg/lookup/using33.C: New.
* g++.dg/lookup/using34.C: New.
* g++.dg/lookup/using35.C: New.
* g++.dg/debug/using4.C: New.
* g++.dg/debug/using5.C: New.
* g++.dg/cpp0x/forw_enum10.C: New.
* g++.old-deja/g++.other/using1.C: Adjust.
* g++.dg/template/using2.C: Likewise.
gcc/cp/ChangeLog
2011-09-15 Fabien Chêne
PR c++/26256
* search.c (lookup_field_1): Get rid of the comment saying that
USING_DECL should not be returned, and actually return USING_DECL
if appropriate.
* semantics.c (finish_member_declaration): Remove the check that
prevents USING_DECLs from being verified by pushdecl_class_level.
* typeck.c (build_class_member_access_expr): Handle USING_DECLs.
* class.c (check_field_decls): Keep using declarations.
* parser.c (cp_parser_nonclass_name): Handle USING_DECLs.
* decl.c (start_enum): Call xref_tag whenever possible.
* name-lookup.c (strip_using_decl): New function.
(supplement_binding_1): Call strip_using_decl on decl and
bval. Perform most of the checks with USING_DECLs stripped. Also
check that the target decl and the target bval does not refer to
the same declaration. Add a hack for handling ENUMERAL_TYPE.
(push_class_level_binding): Call strip_using_decl on decl and
bval. Perform most of the checks with USING_DECLs stripped. Return
true if both decl and bval refer to USING_DECLs and are dependent.
--
Fabien
pr26256.patch
Description: Binary data
Re: [Patch] PR c++/26256
On 06/15/2011 01:58 PM, Fabien Chêne wrote: Otherwise, perhaps that it would be better if the second declaration of E1 does not rely on supplement_binding_1. What do you think ? I agree. We should be using xref_tag for this like we do with classes, so we don't try to push the same tag more than once. Jason
Re: [Patch] PR c++/26256
Hi,
After updating the patch, I now see a failure on c++0x forward enums.
Consider the below code:
template
struct S1
{
enum class E1;
enum class E1;
};
Currently, the second declaration of E1 relies on a successfull call
to supplement_binding_1.
With the patch I am working on, it no longer succeeds, because a new
check is needed, in order to fail if (target_)bval is a type (nearly
similar to the fix for c++/48010).
if (TREE_CODE (target_decl) == TYPE_DECL && DECL_ARTIFICIAL (target_decl)
&& (TREE_CODE (target_bval) != TYPE_DECL
I don't really see a good solution to make it work.
As I already did for c++/48010, we can add the below check...
|| same_type_p (TREE_TYPE (target_decl), TREE_TYPE (target_bval)
But, same_type_p does not returns true, and it seems difficult to make
it return true because the underlying type of the DECL is not yet set
when calling pushtag1 in start_enum.
Otherwise, perhaps that it would be better if the second declaration
of E1 does not rely on supplement_binding_1.
What do you think ?
--
Fabien
Re: [Patch] PR c++/26256
2011/3/5 Jason Merrill :
> On 03/04/2011 03:11 AM, Fabien Chêne wrote:
>>
>> Hmm, I've implemented what you were suggesting, and I don't understand
>> the following check in supplement_binding:
>>
>> else if (TREE_CODE (bval) == TYPE_DECL&& DECL_ARTIFICIAL (bval))
>> {
>> /* The old binding was a type name. It was placed in
>> VALUE field because it was thought, at the point it was
>> declared, to be the only entity with such a name. Move the
>> type name into the type slot; it is now hidden by the new
>> binding. */
>> binding->type = bval;
>> binding->value = decl;
>> binding->value_is_inherited = false;
>> }
>>
>> Why is it usefull ? It prevents the following illegal code from being
>> rejected:
>>
>> struct A
>> {
>> struct type {};
>> typedef int type;
>> };
>
> That's a bug. I guess the check above needs to make sure that decl is not a
> TYPE_DECL.
OK, FYI I have opened PR c++/48010 for this bug, which I am going to fix first.
--
Fabien
Re: [Patch] PR c++/26256
On 03/04/2011 03:11 AM, Fabien Chêne wrote:
Hmm, I've implemented what you were suggesting, and I don't understand
the following check in supplement_binding:
else if (TREE_CODE (bval) == TYPE_DECL&& DECL_ARTIFICIAL (bval))
{
/* The old binding was a type name. It was placed in
VALUE field because it was thought, at the point it was
declared, to be the only entity with such a name. Move the
type name into the type slot; it is now hidden by the new
binding. */
binding->type = bval;
binding->value = decl;
binding->value_is_inherited = false;
}
Why is it usefull ? It prevents the following illegal code from being rejected:
struct A
{
struct type {};
typedef int type;
};
That's a bug. I guess the check above needs to make sure that decl is
not a TYPE_DECL.
Jason
Re: [Patch] PR c++/26256
Hi,
Sorry for the looong delay,
2010/12/22 Jason Merrill :
> On 12/20/2010 07:02 AM, Fabien Chêne wrote:
>>
>> + {
>> + tree using_decl = USING_DECL_DECLS (field);
>> + if ((TREE_CODE (using_decl) == FIELD_DECL
>> + || TREE_CODE (using_decl) == TYPE_DECL)
>> + && DECL_NAME (using_decl) == name)
>> + return using_decl;
>> + continue;
>> + }
>
> I think if a USING_DECL designates a function, it shouldn't be in
> TYPE_FIELDS. And if we do need any code for handling USING_DECL here, don't
> we need it for the sorted case above as well?
>
>> - if (TREE_CODE (decl) == TYPE_DECL && DECL_ARTIFICIAL (decl))
>> + if (TREE_CODE (decl) == TYPE_DECL && DECL_ARTIFICIAL (decl)
>> + && (TREE_CODE (bval) != USING_DECL
>> + || (!DECL_DEPENDENT_P (bval)
>> + && (TREE_CODE (USING_DECL_DECLS (bval)) != TYPE_DECL
>
> Why is this change needed? Why is the unconditional setting of
> binding->type when we see a nested class no longer appropriate?
>
> It seems to me that instead of special-casing using-declarations in
> supplement_binding, we want the code that currently checks the tree code of
> decl and bval to look through USING_DECLs. Likewise in
> push_class_level_binding.
>
> How are using-declarations combined with normal declarations?
Hmm, I've implemented what you were suggesting, and I don't understand
the following check in supplement_binding:
else if (TREE_CODE (bval) == TYPE_DECL && DECL_ARTIFICIAL (bval))
{
/* The old binding was a type name. It was placed in
VALUE field because it was thought, at the point it was
declared, to be the only entity with such a name. Move the
type name into the type slot; it is now hidden by the new
binding. */
binding->type = bval;
binding->value = decl;
binding->value_is_inherited = false;
}
Why is it usefull ? It prevents the following illegal code from being rejected:
struct A
{
struct type {};
typedef int type;
};
Hence, the same now happens with using declarations.
--
Fabien
