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 09:42:00 -0000 [thread overview]
Message-ID: <20120926094227.GA4335@adacore.com> (raw)
In-Reply-To: <20120926132621.5f45acb7@spoyarek>
> @@ -637,7 +637,7 @@ amd64_return_value (struct gdbarch *gdba
> }
>
> gdb_assert (class[1] != AMD64_MEMORY);
> - gdb_assert (len <= 16);
> + gdb_assert (TYPE_LENGTH (type) <= 16);
>
> for (i = 0; len > 0; i++, len -= 8)
> {
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)
> Index: gdb/cris-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/cris-tdep.c,v
> retrieving revision 1.185
> diff -u -p -r1.185 cris-tdep.c
> --- gdb/cris-tdep.c 18 May 2012 21:02:47 -0000 1.185
> +++ gdb/cris-tdep.c 26 Sep 2012 07:45:40 -0000
> @@ -1662,20 +1662,20 @@ cris_store_return_value (struct type *ty
> struct gdbarch *gdbarch = get_regcache_arch (regcache);
> enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> ULONGEST val;
> - int len = TYPE_LENGTH (type);
>
> - if (len <= 4)
> + if (TYPE_LENGTH (type) <= 4)
> {
> /* Put the return value in R10. */
> - val = extract_unsigned_integer (valbuf, len, byte_order);
> + val = extract_unsigned_integer (valbuf, TYPE_LENGTH (type), byte_order);
> regcache_cooked_write_unsigned (regcache, ARG1_REGNUM, val);
> }
> - else if (len <= 8)
> + else if (TYPE_LENGTH (type) <= 8)
> {
> /* Put the return value in R10 and R11. */
> val = extract_unsigned_integer (valbuf, 4, byte_order);
> regcache_cooked_write_unsigned (regcache, ARG1_REGNUM, val);
> - val = extract_unsigned_integer ((char *)valbuf + 4, len - 4, byte_order);
> + val = extract_unsigned_integer ((char *)valbuf + 4,
> + TYPE_LENGTH (type) - 4, byte_order);
> regcache_cooked_write_unsigned (regcache, ARG2_REGNUM, val);
> }
> else
> @@ -1833,21 +1833,21 @@ cris_extract_return_value (struct type *
> struct gdbarch *gdbarch = get_regcache_arch (regcache);
> enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> ULONGEST val;
> - int len = TYPE_LENGTH (type);
>
> - if (len <= 4)
> + if (TYPE_LENGTH (type) <= 4)
> {
> /* Get the return value from R10. */
> regcache_cooked_read_unsigned (regcache, ARG1_REGNUM, &val);
> - store_unsigned_integer (valbuf, len, byte_order, val);
> + store_unsigned_integer (valbuf, TYPE_LENGTH (type), byte_order, val);
> }
> - else if (len <= 8)
> + else if (TYPE_LENGTH (type) <= 8)
> {
> /* Get the return value from R10 and R11. */
> regcache_cooked_read_unsigned (regcache, ARG1_REGNUM, &val);
> store_unsigned_integer (valbuf, 4, byte_order, val);
> regcache_cooked_read_unsigned (regcache, ARG2_REGNUM, &val);
> - store_unsigned_integer ((char *)valbuf + 4, len - 4, byte_order, val);
> + store_unsigned_integer ((char *)valbuf + 4, TYPE_LENGTH (type) - 4,
> + byte_order, val);
> }
> else
> error (_("cris_extract_return_value: type length too large"));
Why is it better to repeat the use of TYPE_LENGTH rather than use
a single variable? It's definitely not obvious to me, and it seems
even simpler to just change the type of variable "len"... This patch
feels like a step backwards, and trying to reduce the size of a patch
would be the wrong justification for it.
--
Joel
next prev parent reply other threads:[~2012-09-26 9:42 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 [this message]
2012-09-26 9:58 ` Siddhesh Poyarekar
2012-09-26 10:52 ` Joel Brobecker
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=20120926094227.GA4335@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