Re: [PATCH] Eliminate local variables to use TYPE_LENGTH directly

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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