This is the mail archive of the mailing list for the GDB project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

>>>>> "Sami" == sami wagiaalla <> 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

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.

Sami> +	  /* (b) pointer to ancestor-pointer conversion.  */
Sami> +	  if (is_public_ancestor (parm, arg))

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

Also, why specifically is_public_ancestor and not is_ancestor?

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.  This is fine, even great, but it is handy for
reviewers if you note that sort of thing in the patch explanation.

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;

Do the new tests cover this case?  I could not immediately tell.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]