This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Eliminate local variables to use TYPE_LENGTH directly
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: Siddhesh Poyarekar <siddhesh at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 25 Sep 2012 10:25:58 +0200
- Subject: Re: [PATCH] Eliminate local variables to use TYPE_LENGTH directly
- References: <20120924173542.625d322c@spoyarek>
Hi Siddhesh,
On Mon, 24 Sep 2012 14:05:42 +0200, Siddhesh Poyarekar wrote:
[...]
> --- ax-gdb.c 19 Jul 2012 15:38:16 -0000 1.105
> +++ ax-gdb.c 24 Sep 2012 09:16:37 -0000
> @@ -367,9 +367,7 @@
> {
> case axs_lvalue_memory:
> {
> - int length = TYPE_LENGTH (check_typedef (value.type));
> -
> - ax_const_l (ax, length);
> + ax_const_l (ax, TYPE_LENGTH (check_typedef (value.type)));
While correct I find this expression too tricky, check_typedef should be
called rather ahead of some block of code later depending on it. I would
choose:
/* Initialize TYPE_LENGTH of it. */
check_typedef (value.type);
ax_const_l (ax, TYPE_LENGTH (value.type));
(You even did so in s390_value_from_register as I see.)
> ax_simple (ax, aop_trace);
> }
> break;
> @@ -425,8 +423,6 @@
>
> case axs_lvalue_memory:
> {
> - int length = TYPE_LENGTH (check_typedef (value->type));
> -
Again here, similar to above:
/* Initialize TYPE_LENGTH of it. */
check_typedef (value->type);
> if (string_trace)
> ax_simple (ax, aop_dup);
>
> @@ -435,7 +431,7 @@
> "const8 SIZE trace" is also three bytes, does the same
> thing, and the simplest code which generates that will also
> work correctly for objects with large sizes. */
> - ax_const_l (ax, length);
> + ax_const_l (ax, TYPE_LENGTH (check_typedef (value->type)));
and:
ax_const_l (ax, TYPE_LENGTH (value->type));
> ax_simple (ax, aop_trace);
>
> if (string_trace)
[...]
> --- stack.c 19 Jul 2012 15:33:25 -0000 1.256
> +++ stack.c 24 Sep 2012 09:16:42 -0000
[...]
> @@ -373,12 +374,12 @@
>
> TRY_CATCH (except, RETURN_MASK_ERROR)
> {
> - unsigned len_deref;
> + struct type *t;
A nitpick but here is already 'type' and now also 't', please rename 't' to
for example 'type_deref'.
>
> val_deref = coerce_ref (val);
> if (value_lazy (val_deref))
> value_fetch_lazy (val_deref);
> - len_deref = TYPE_LENGTH (value_type (val_deref));
> + t = value_type (val_deref);
>
> entryval_deref = coerce_ref (entryval);
> if (value_lazy (entryval_deref))
> @@ -389,7 +390,7 @@
> if (val != val_deref
> && value_available_contents_eq (val_deref, 0,
> entryval_deref, 0,
> - len_deref))
> + TYPE_LENGTH (t)))
It will not fit here then but you can indent the line left just to keep it to
80 columns in such case (the columns will not align):
TYPE_LENGTH (type_deref)))
> val_equal = 1;
> }
>
> Index: tracepoint.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/tracepoint.c,v
> retrieving revision 1.266
> diff -u -r1.266 tracepoint.c
> --- tracepoint.c 18 Sep 2012 12:09:26 -0000 1.266
> +++ tracepoint.c 24 Sep 2012 09:16:43 -0000
> @@ -1456,7 +1456,7 @@
> }
> else
> {
> - unsigned long addr, len;
> + unsigned long addr;
> struct cleanup *old_chain = NULL;
> struct cleanup *old_chain1 = NULL;
>
> @@ -1486,8 +1486,9 @@
> /* Safe because we know it's a simple expression. */
> tempval = evaluate_expression (exp);
> addr = value_address (tempval);
> - len = TYPE_LENGTH (check_typedef (exp->elts[1].type));
> - add_memrange (collect, memrange_absolute, addr, len);
Maybe add a line here:
/* Initialize TYPE_LENGTH of it. */
> + check_typedef (exp->elts[1].type);
> + add_memrange (collect, memrange_absolute, addr,
> + TYPE_LENGTH (exp->elts[1].type));
> break;
>
> case OP_VAR_VALUE:
[...]
I see no bugs there, OK for check-in with those few changes.
Thanks,
Jan