[patch 2/2] Fix overload resolution of int* vs void*

sami wagiaalla swagiaal@redhat.com
Tue Oct 12 20:01:00 GMT 2010


On 10/08/2010 06:54 PM, Tom Tromey wrote:
>>>>>> "Sami" == sami wagiaalla<swagiaal@redhat.com>  writes:
>
> Sami>  @@ -1886,6 +1886,14 @@ do_is_ancestor (struct type *base, struct type *dclass, int public)
> Sami>     if (class_types_same_p (base, dclass))
> Sami>       return 1;
>
> Sami>  +  if ((TYPE_CODE (base) == TYPE_CODE_PTR
> Sami>  +&&  TYPE_CODE (dclass) == TYPE_CODE_PTR)
> Sami>  +       || (TYPE_CODE (base) == TYPE_CODE_REF
> Sami>  +	&&  TYPE_CODE (dclass) == TYPE_CODE_REF))
> Sami>  +    return do_is_ancestor (TYPE_TARGET_TYPE (base),
> Sami>  +                           TYPE_TARGET_TYPE (dclass),
> Sami>  +                           public);
>
> This seems strange to me.  It ought to only be applicable at the
> outermost call.  But in that case, I think a wrapper function would be
> more appropriate, so that do_is_ancestor has a single purpose.
>
> Also, is this the right thing for the caller when there are multiple
> pointers (like "Class ***")?  Or do you intend to only strip a single
> pointer?
>

I understood that one could convert 'Class ***' to 'BaseClass ***', but 
it turns out that is in correct. I should not have assumed that without 
testing, and there is nothing in the spec that should have made me think 
so. Since that is the case, the calling function can just deference the 
pointers.

> Sami>  +  /* Resolve typedefs */
> Sami>  +  if (TYPE_CODE (a) == TYPE_CODE_TYPEDEF)
> Sami>  +    a = check_typedef (a);
> Sami>  +  if (TYPE_CODE (b) == TYPE_CODE_TYPEDEF)
> Sami>  +    b = check_typedef (b);
>
> It is ok to call check_typedef unconditionally, but I see this is just
> copied from rank_one_type.
>

Yeah I wondered about that when I copied the code but could not think of 
a reason not to do so.

I added a test case for the typedef scenario.

> Sami>  +	  /* (b) pointer to ancestor-pointer conversion.  */
> Sami>  +	  if (is_public_ancestor (parm, arg))
> Sami>  +	    return BASE_PTR_CONVERSION_BADNESS;
>
> I am curious why you didn't just give POINTER_CONVERSION_BADNESS a new
> value and instead introduced BASE_PTR_CONVERSION_BADNESS.
>
> But then, I also don't understand the existing code that returns
> POINTER_CONVERSION_BADNESS...
>

base pointer conversion (BASE_PTR_CONVERSION_BADNESS) is meant to be a 
slightly better option than generic (POINTER_CONVERSION_BADNESS)

> Also, why specifically is_public_ancestor and not is_ancestor?
>

You can convert a pointer to B to a pointer to A only if A is an 
accessible ancestor of B.

> I suppose this is an incremental patch -- still not truly correct with
> regards to the distance-to-base problem, but an improvement over what
> gdb currently does.

Yes. I am working on a second patch to solve the distance to base 
problem. I will post it in reply to my previous attempt at that problem.

   This is fine, even great, but it is handy for
> reviewers if you note that sort of thing in the patch explanation.
>

Cool, will do in the future. Incidentally, I should mention that is 
there is also the conversion of pointer to other data types and ranking 
within them that needs to be covered.

> Sami>   	case TYPE_CODE_ARRAY:
> Sami>  -	  return rank_one_type (TYPE_TARGET_TYPE (parm),
> Sami>  -				TYPE_TARGET_TYPE (arg));
> Sami>  +	  if (types_equal (TYPE_TARGET_TYPE (parm),
> Sami>  +	                   TYPE_TARGET_TYPE (arg)))
> Sami>  +	    return 0;
> Sami>  +	  return INCOMPATIBLE_TYPE_BADNESS;
>
> Do the new tests cover this case?  I could not immediately tell.
>

Yes, take a look at foo1_2 foo2_2, and foo2_4. Let me know if the tests 
need elaborations it is easy to miss cases when you are so close to the 
code.

Revised patch attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: overload_voidp.patch
Type: text/x-patch
Size: 13277 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20101012/0f97d9c1/attachment.bin>


More information about the Gdb-patches mailing list