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