From: Joel Brobecker <brobecker@adacore.com>
To: Siddhesh Poyarekar <siddhesh@redhat.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [commit][obv] Use TYPE_LENGTH directly where possible
Date: Wed, 26 Sep 2012 10:52:00 -0000 [thread overview]
Message-ID: <20120926105205.GB4335@adacore.com> (raw)
In-Reply-To: <20120926152740.0900e9b7@spoyarek>
> > 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
next prev parent reply other threads:[~2012-09-26 10:52 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-26 7:57 Siddhesh Poyarekar
2012-09-26 9:42 ` Joel Brobecker
2012-09-26 9:58 ` Siddhesh Poyarekar
2012-09-26 10:52 ` Joel Brobecker [this message]
2012-09-26 14:59 ` Pedro Alves
2012-09-26 15:09 ` Siddhesh Poyarekar
2012-09-26 15:07 ` Tom Tromey
2012-09-28 4:00 ` Mark Kettenis
2012-09-28 8:11 ` Jan Kratochvil
2012-09-27 9:59 ` Joel Brobecker
2012-09-27 10:13 ` Siddhesh Poyarekar
2012-09-27 10:19 ` Joel Brobecker
2012-09-27 10:42 ` Siddhesh Poyarekar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120926105205.GB4335@adacore.com \
--to=brobecker@adacore.com \
--cc=gdb-patches@sourceware.org \
--cc=siddhesh@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox