This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 09/26/2012 11:52 AM, Joel Brobecker wrote:
>>> 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 agree with Joel, and was also silently glumbling.  :-)

> 
>> 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).

-- 
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]