From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19594 invoked by alias); 25 Sep 2012 08:26:25 -0000 Received: (qmail 19578 invoked by uid 22791); 25 Sep 2012 08:26:22 -0000 X-SWARE-Spam-Status: No, hits=-6.7 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 25 Sep 2012 08:26:05 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q8P8Q4F0010500 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 25 Sep 2012 04:26:04 -0400 Received: from host2.jankratochvil.net (ovpn-113-58.phx2.redhat.com [10.3.113.58]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q8P8Pwkf030783 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Tue, 25 Sep 2012 04:26:01 -0400 Date: Tue, 25 Sep 2012 08:26:00 -0000 From: Jan Kratochvil To: Siddhesh Poyarekar Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Eliminate local variables to use TYPE_LENGTH directly Message-ID: <20120925082558.GA9305@host2.jankratochvil.net> References: <20120924173542.625d322c@spoyarek> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120924173542.625d322c@spoyarek> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-09/txt/msg00540.txt.bz2 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