diff mbox

PR c++/26256

Message ID AANLkTin5FvEzTYUg+W5bRzk462LaJ4hpk3s+yxZzF_RY@mail.gmail.com
State New
Headers show

Commit Message

Fabien Chêne Nov. 17, 2010, 10:47 a.m. UTC
2010/11/15 Fabien Chêne <fabien.chene@gmail.com>:
[...]

Comments

Fabien Chêne Dec. 20, 2010, 12:02 p.m. UTC | #1
2010/11/17 Fabien Chêne <fabien.chene@gmail.com>:
> 2010/11/15 Fabien Chêne <fabien.chene@gmail.com>:
> [...]
>
> Index: gcc/cp/typeck.c
> ===================================================================
> --- gcc/cp/typeck.c     (revision 166743)
> +++ gcc/cp/typeck.c     (working copy)
> @@ -2340,6 +2340,11 @@ build_class_member_access_expr (tree obj
>        result = build2 (COMPOUND_EXPR, TREE_TYPE (result),
>                         object, result);
>     }
> +  else if (TREE_CODE (member) == USING_DECL)
> +       result = build_class_member_access_expr (object,
> +                                               USING_DECL_DECLS (member),
> +                                               access_path, preserve_reference,
> +                                               complain);
>
> I guess it would be safer to also check !DECL_DEPENDENT_P (member), I
> will update the patch.

Done in the attached patch, no regressions on
x86_64-unknown-linux-gnu. OK to queue it for next stage 1 ?
Jason Merrill Dec. 22, 2010, 10:21 p.m. UTC | #2
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?

Jason
Fabien Chêne March 4, 2011, 8:11 a.m. UTC | #3
Hi,

Sorry for the looong delay,

2010/12/22 Jason Merrill <jason@redhat.com>:
> 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.
Jason Merrill March 5, 2011, 8:07 p.m. UTC | #4
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
Fabien Chêne March 8, 2011, 9:48 p.m. UTC | #5
2011/3/5 Jason Merrill <jason@redhat.com>:
> 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 Chêne June 15, 2011, 5:58 p.m. UTC | #6
Hi,

After updating the patch, I now see a failure on c++0x forward enums.
Consider the below code:

template <class T>
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 ?
Jason Merrill June 22, 2011, 3:36 p.m. UTC | #7
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
Fabien Chêne Sept. 17, 2011, 1:44 p.m. UTC | #8
Hi!

(Again, sorry for the long delay, but I indeed have very few time to work on it)

2011/6/22 Jason Merrill <jason@redhat.com>:
> 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<typename T> struct S1
{
    enum E : T;   // { dg-error "previous definition" }
    enum E : int;     // { dg-error "different underlying type" }
};
template struct S1<short>; // { 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  <fabien@gcc.gnu.org>

	PR c++/26256
	* dbxout.c (dbxout_type_fields): Ignore using declarations.


gcc/testsuite/ChangeLog

2011-09-15  Fabien Chêne  <fabien@gcc.gnu.org>

	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  <fabien@gcc.gnu.org>

	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.
Jason Merrill Sept. 20, 2011, 12:02 a.m. UTC | #9
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?
Fabien Chêne Sept. 21, 2011, 5:59 p.m. UTC | #10
Hi,

2011/9/20 Jason Merrill <jason@redhat.com>:
> 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 Chêne Sept. 21, 2011, 6:01 p.m. UTC | #11
... with the ChangeLog

gcc/ChangeLog

2011-09-21  Fabien Chêne  <fabien@gcc.gnu.org>

	PR c++/26256
	* dbxout.c (dbxout_type_fields): Ignore using declarations.


gcc/testsuite/ChangeLog

2011-09-21  Fabien Chêne  <fabien@gcc.gnu.org>

	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  <fabien@gcc.gnu.org>

	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.
Jason Merrill Sept. 21, 2011, 6:10 p.m. UTC | #12
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
Fabien Chêne Sept. 22, 2011, 8:22 a.m. UTC | #13
2011/9/21 Jason Merrill <jason@redhat.com>:
> 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 ?
Jason Merrill Sept. 22, 2011, 3:45 p.m. UTC | #14
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
Fabien Chêne Sept. 22, 2011, 9:11 p.m. UTC | #15
2011/9/22 Jason Merrill <jason@redhat.com>:
> 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,
Jason Merrill Sept. 22, 2011, 10:33 p.m. UTC | #16
On 09/22/2011 05:11 PM, Fabien Chêne wrote:
> 2011/9/22 Jason Merrill<jason@redhat.com>:

>> 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
Fabien Chêne Sept. 23, 2011, 7:52 a.m. UTC | #17
2011/9/23 Jason Merrill <jason@redhat.com>:
> On 09/22/2011 05:11 PM, Fabien Chêne wrote:
>>
>> 2011/9/22 Jason Merrill<jason@redhat.com>:
>
>>> 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 Chêne Sept. 25, 2011, 7:54 p.m. UTC | #18
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  <fabien@gcc.gnu.org>

	PR c++/26256
	* dbxout.c (dbxout_type_fields): Ignore using declarations.


gcc/testsuite/ChangeLog

2011-09-21  Fabien Chêne  <fabien@gcc.gnu.org>

	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  <fabien@gcc.gnu.org>

	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.
Paolo Carlini Sept. 25, 2011, 8:07 p.m. UTC | #19
... nitpicking, of course, but in the testcases you have many blank trailing lines (and also some gratuitus, imho, blank lines in the middle)

Paolo
diff mbox

Patch

Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 166743)
+++ gcc/cp/typeck.c	(working copy)
@@ -2340,6 +2340,11 @@  build_class_member_access_expr (tree obj
 	result = build2 (COMPOUND_EXPR, TREE_TYPE (result),
 			 object, result);
     }
+  else if (TREE_CODE (member) == USING_DECL)
+       result = build_class_member_access_expr (object,
+						USING_DECL_DECLS (member),
+						access_path, preserve_reference,
+						complain);

I guess it would be safer to also check !DECL_DEPENDENT_P (member), I
will update the patch.