Re: RFA: shrink main_type

[Joel Brobecker <brobecker@adacore.com>]

:REVIEWMAIL:

> 2008-08-18  Tom Tromey  <tromey@redhat.com>

(wow, I'm surprised you actually typed the entire changelog).
For the future, I think that saying something like this:

        * gdbtype.h (...): bla bla bla.
        * xml-tdesc.c: Update throughout.

Is OK. I remember reading something like this in the GNU Coding Standards.

Overall, the patch looks OK to me.  I like how it simplifies the code
a little bit. I also like the fact that you separated the type flags and
the instance flags, and added INSTANCE to the enum name.

Most of the rest of the patch is fairly mechanical, and looked fine.
I think there is one incorrect change, however, inside copy_type_recursive:

> @@ -2933,24 +2973,19 @@ copy_type_recursive (struct objfile *objfile,
>    stored->new = new_type;
>    *slot = stored;
>  
> -  /* Copy the common fields of types.  */
> -  TYPE_CODE (new_type) = TYPE_CODE (type);
> -  TYPE_ARRAY_UPPER_BOUND_TYPE (new_type) = 
> -    TYPE_ARRAY_UPPER_BOUND_TYPE (type);
> -  TYPE_ARRAY_LOWER_BOUND_TYPE (new_type) = 
> -    TYPE_ARRAY_LOWER_BOUND_TYPE (type);
> +  /* Copy the common fields of types.  For the main type, we simply
> +     copy the entire thing and then update specific fields as needed.  */
> +  memcpy (TYPE_MAIN_TYPE (new_type), TYPE_MAIN_TYPE (type),
> +	  sizeof (struct main_type));
>    if (TYPE_NAME (type))
>      TYPE_NAME (new_type) = xstrdup (TYPE_NAME (type));
>    if (TYPE_TAG_NAME (type))
>      TYPE_TAG_NAME (new_type) = xstrdup (TYPE_TAG_NAME (type));
> -  TYPE_FLAGS (new_type) = TYPE_FLAGS (type);
> -  TYPE_VPTR_FIELDNO (new_type) = TYPE_VPTR_FIELDNO (type);
>  
>    TYPE_INSTANCE_FLAGS (new_type) = TYPE_INSTANCE_FLAGS (type);
>    TYPE_LENGTH (new_type) = TYPE_LENGTH (type);
>  
>    /* Copy the fields.  */
> -  TYPE_NFIELDS (new_type) = TYPE_NFIELDS (type);
>    if (TYPE_NFIELDS (type))
>      {
>        int i, nfields;

Because of the memcpy, you end up copying over the objfile field,
which is incorrect, since this code is used to duplicate type
information that is independent of the given objfile. I think there
are other fields as well that are now implicitly copied (such as
vptr_fieldno, and type_specific), but I think that in this case
it's not harmful.

>    if (ada_is_packed_array_type (type0)  /* revisit? */
> -      || (TYPE_FLAGS (type0) & TYPE_FLAG_FIXED_INSTANCE))
> +      || (TYPE_FIXED_INSTANCE (type0)))
>      return type0;

Major nit-picking: The '('/')' around TYPE_FIXED_INSTANCE (type0) is
no longer necessary. I won't hate you if you don't fix that, but
I do personally like it better without.

> -  if (len == 0 && (TYPE_FLAGS (type) & TYPE_FLAG_STUB) != 0)
> +  if (len == 0 && (TYPE_STUB (type)) != 0)
>      return -1;

Similarly, I thin it would be more readable to have:

    if (len == 0 && TYPE_STUB (type))

(again, that might be a personal preference, feel free to disagree)

I spotted a few more areas where I could make the same type of comment:

> diff --git a/gdb/iq2000-tdep.c b/gdb/iq2000-tdep.c
> index 4843ff1..8581aee 100644
> --- a/gdb/iq2000-tdep.c
> +++ b/gdb/iq2000-tdep.c
> @@ -93,7 +93,7 @@ iq2000_pointer_to_address (struct type * type, const gdb_byte * buf)
>  
>    if (target == TYPE_CODE_FUNC
>        || target == TYPE_CODE_METHOD
> -      || (TYPE_FLAGS (TYPE_TARGET_TYPE (type)) & TYPE_FLAG_CODE_SPACE) != 0)
> +      || (TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type))) != 0)


> --- a/gdb/sh-tdep.c
> +++ b/gdb/sh-tdep.c
> @@ -1080,7 +1080,7 @@ sh_push_dummy_call_fpu (struct gdbarch *gdbarch,
>       non-vararg argument to be on the stack, no matter how many
>       registers have been used so far.  */
>    if (sh_is_renesas_calling_convention (func_type)
> -      && (TYPE_FLAGS (func_type) & TYPE_FLAG_VARARGS))
> +      && (TYPE_VARARGS (func_type)))
>      last_reg_arg = TYPE_NFIELDS (func_type) - 2;
>  
>    /* first force sp to a 4-byte alignment */
> @@ -1217,7 +1217,7 @@ sh_push_dummy_call_nofpu (struct gdbarch *gdbarch,
>       non-vararg argument to be on the stack, no matter how many
>       registers have been used so far.  */
>    if (sh_is_renesas_calling_convention (func_type)
> -      && (TYPE_FLAGS (func_type) & TYPE_FLAG_VARARGS))
> +      && (TYPE_VARARGS (func_type)))
>      last_reg_arg = TYPE_NFIELDS (func_type) - 2;

The testcase update looked fine too, but was very difficult to read.
I didn't see anything wrong, so between you and I, it should be fine.

-- 
Joel