This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
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