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

[Mark Kettenis <mark dot kettenis at xs4all dot nl>]

> Date: Wed, 26 Sep 2012 12:52:06 +0200
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > > 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.
> 
> OK. I see why it works.
> 
> But I can definitely see someone like myself missing that subtlety,
> and commit an obvious change that reduces the duplication by re-using
> the variable in the gdb_assert call. To the unattentive me, that's
> an obvious improvement. I think that's an issue.
> 
> > 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 know it makes no functional difference, but it does make a noticeable
> difference in terms of maintenance, IMO. And I have in fact been
> silently grumbling about your patches being labeled "obvious" when
> in fact I do not consider them obvious. But I have let them go, because
> it wasn't significant enough that I felt I needed to talk about it.
> 
> But this part of this patch in particular did catch my attention, and
> I feel that we need to discuss it. I think this goes in the wrong
> direction, and it would be better to just change the variable
> (constant???) type, rather than duplicate the expression everywhere.
> I don't have a strong opinion on this, and if other maintainers are ok
> with it, then OK. But in the meantime, I think the previous version
> was better.
> 
> > 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.
> 
> I agree it's subjective. Just FYI, my tolerance starts at 2.  If I need
> to repeat an expression, I often start thinking about factorizing into
> constants, functions, etc (duplication is not the only part of the
> decision process, so I don't necessarily do it).

I agree with Joel.  Actually my tolerance starts at 1, if it avoids
having lines that are too long or if it reduces the number of nested
parentheses to a more manageable level.