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


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

-- 
Joel


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