Re: [PATCH PR gdb/16841] virtual inheritance via typedef cannot find base

[Wei-min Pan <weimin.pan@oracle.com>]

Hi Tom,

Thanks very much for the comments.

On 7/28/2018 7:28 PM, Tom Tromey wrote:
> Thanks for the patch.  There are a few small issues with it.
>
> First, git says that most of the lines in the valops.c part have a
> trailing space.  Please remove those.

Will do.

> Weimin> +		  for (index = 0; index < TYPE_NFIELDS (domain); index++)
>
> I think the loop limit should be TYPE_N_BASECLASSES.

OK, will change it to TYPE_N_BASECLASSES. BTW, there are quite a number 
of places that use:

     for (i = 0; i < TYPE_NFIELDS (type); ++i)

> And, is it possible for the desired type to be a base class of a base class?
> I suspect so, but I didn't try to check; in this case you will need some
> recursion here, and also I think a new test for this case would be good.

Yes, I think it's possible and will need to do recursions here. Also 
will expand the submitted
test with such classes.

>
> Weimin> +		      if (TYPE_FIELDS (domain)[index].type == curtype)
>
> It is more idiomatic to write TYPE_FIELD_TYPE (domain, index) here.
> Also I think you need check_typedef on the left-hand-side.
> Otherwise perhaps using a typedef for a base class would confuse gdb
> (depending on whether the compiler emits typedefs here or not).

Good point. Will change it to

     if (check_typedef (TYPE_FIELD_TYPE (domain,index)) == curtype)

> Weimin> +		  else
> Weimin> +		      mem_offset = value_as_long (ptr);
>
> This line is indented too far.

OK.

Thanks again,
Weimin

>
> thanks,
> Tom