Re: [commit][obv] Use TYPE_LENGTH directly where possible

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Wed, 26 Sep 2012 11:42:27 +0200, Joel wrote:
> > @@ -637,7 +637,7 @@ amd64_return_value (struct gdbarch *gdba
> >      }
> >  
> >    gdb_assert (class[1] != AMD64_MEMORY);
> > -  gdb_assert (len <= 16);
> > +  gdb_assert (TYPE_LENGTH (type) <= 16);
> >  
> >    for (i = 0; len > 0; i++, len -= 8)
> >      {
> 
> Why is the type not OK for the assert, and yet OK for the rest of
> the code? (the same question applies to other files, as well)

This is so that the assert is not subject to any truncation/overflow
resulting due to the type of LEN.  That way, I don't have to expand LEN
since I know that the value is always going to be less than 16 and if
something actually goes wrong, then the assert will definitely catch it.

> Why is it better to repeat the use of TYPE_LENGTH rather than use
> a single variable? It's definitely not obvious to me, and it seems
> even simpler to just change the type of variable "len"... This patch
> feels like a step backwards, and trying to reduce the size of a patch
> would be the wrong justification for it.
> 

It does not make any functional difference at all and the justification
is in fact that it reduces the size of the bitpos patch.  I have
committed similar changes in the past that were deemed to be OK, so I
don't see why this patch in particular should be a problem.

I have not made changes in places where more than 4-5 substitutions
were necessary and where the code started looking unwieldy as a
result.  I guess both those parameters are subjective, but I couldn't
see a coding convention that seems to have been strictly followed
throughout the code base.

Regards,
Siddhesh