Re: [patch] Re: Regression: field type preservation: 7.0 -> 7.0.1+HEAD

[Joel Brobecker <brobecker@adacore.com>]

> gdb/
> 2010-01-02  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* value.c (value_primitive_field): Remove one check_typedef call.
> 	Move bitpos and container_bitsize initialization after
> 	allocate_value_lazy.  New comment before accessing TYPE_LENGTH.

Interesting. This patch allowed me to figure out why the initial patch
sent by Vladimir was in fact working when it looks like it could not
possibly work :).

Personally, I'm not a big fan of the approach you took in this case.
You rely on a side-effect of the various allocate_* routines, but
this is only because these routines call check_typedef. In particular,
there is something interesting in allocate_value_lazy. A variable is
declared...

        struct type *atype = check_typedef (type);

... but then never used. There is no available info as to why this is,
as this is here since day 1 of the public CVS repo, but it's now clear
to me that this helps fixing that length - a type length that does not
appear to be used at all in the rest of the routine!

In my opinion, I agree with Daniel's comment that it is unusual to
call check_typedef without storing the function result.  But this
seems to be less awkward than relying on an undocumented side-effect
of a function that uses an undocumented side-effect of a check_typedef,
especially since the function in question does not really need that
side-effect to be applied at the time the function is called!

So, my proposal, if the other maintainers agree, is to document
the side-effect of check_typedef (sets the typedef TYPE_LENGTH)
as this appears to be a fully-intended behavior, and then do:

> -  type = check_typedef (type);

> +  /* Call check_typedef on our type to make sure that, if TYPE
> +     is a TYPE_CODE_TYPEDEF, its length is set to the length
> +     of the target type instead of zero.  However, we do not
> +     replace the typedef type by the target type, because we want
> +     to keep the typedef in order to be able to print the type
> +     description correctly.  */
> +  check_typedef (type);

> gdb/testsuite/
> 2010-01-02  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.mi/var-cmd.c (do_bitfield_tests): Change "V.sharable" type to
> 	"uint_for_mi_testing".

This part is OK.

-- 
Joel