From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1002 invoked by alias); 13 Sep 2011 09:25:11 -0000 Received: (qmail 974 invoked by uid 22791); 13 Sep 2011 09:25:10 -0000 X-SWARE-Spam-Status: No, hits=-6.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,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, 13 Sep 2011 09:24:47 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p8D9OiJe016667 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 13 Sep 2011 05:24:44 -0400 Received: from host1.jankratochvil.net (ovpn-116-38.ams2.redhat.com [10.36.116.38]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p8D9Offg000388 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 13 Sep 2011 05:24:43 -0400 Received: from host1.jankratochvil.net (localhost [127.0.0.1]) by host1.jankratochvil.net (8.14.4/8.14.4) with ESMTP id p8D9OfJB014628; Tue, 13 Sep 2011 11:24:41 +0200 Received: (from jkratoch@localhost) by host1.jankratochvil.net (8.14.4/8.14.4/Submit) id p8D9OeFY014625; Tue, 13 Sep 2011 11:24:40 +0200 Date: Tue, 13 Sep 2011 12:20:00 -0000 From: Jan Kratochvil To: Abhijit Halder Cc: Pedro Alves , gdb-patches@sourceware.org Subject: Re: Some code-cleanup Message-ID: <20110913092440.GA12661@host1.jankratochvil.net> References: <201109121623.04292.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-2022-jp Content-Disposition: inline In-Reply-To: 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: 2011-09/txt/msg00208.txt.bz2 On Mon, 12 Sep 2011 17:30:06 +0200, Abhijit Halder wrote: > Index: gdb/ChangeLog > =================================================================== > RCS file: /cvs/src/src/gdb/ChangeLog,v > retrieving revision 1.13320 > diff -u -p -r1.13320 ChangeLog > --- gdb/ChangeLog 11 Sep 2011 09:54:16 -0000 1.13320 > +++ gdb/ChangeLog 12 Sep 2011 15:25:45 -0000 ChangeLog entries should be sent as text, not as a part of the patch, as during application it usually just causes a conflict as the reader has slightly updated codebase since the post time. > @@ -1,3 +1,19 @@ > +2011-09-12 Abhijit Halder > + > + Code cleanup. > + * gdb/parse.c (write_exp_elt): Change the argument to pass a pointer There should be only "parse.c", as it is in gdb/ChangeLog. > + of union exp_element instead of an element of the same. > + * (write_exp_elt_opcode): Change argument of write_exp_elt call. > + * (write_exp_elt_sym): Change argument of write_exp_elt call. > + * (write_exp_elt_block): Change argument of write_exp_elt call. > + * (write_exp_elt_objfile): Change argument of write_exp_elt call. > + * (write_exp_elt_longcst): Change argument of write_exp_elt call. > + * (write_exp_elt_dblcst): Change argument of write_exp_elt call. > + * (write_exp_elt_decfloatcst): Change argument of write_exp_elt call. > + * (write_exp_elt_type): Change argument of write_exp_elt call. > + * (write_exp_elt_intern): Change argument of write_exp_elt call. In these lines there should not be `* ' as it is not a new file. And the entries for functions in the same file should be merged. See examples in the GNU Coding Style document: (write_exp_elt_opcode, write_exp_elt_sym, write_exp_elt_block) (write_exp_elt_objfile, write_exp_elt_longcst, write_exp_elt_dblcst) (write_exp_elt_decfloatcst, write_exp_elt_type, write_exp_elt_intern): Change argument of write_exp_elt call. > + * src/sim/ppc/dp-bit.c (unpack_d): Change the formatting. Inappropriate here, see at the bottom. > --- gdb/parse.c 17 Jun 2011 20:24:22 -0000 1.110 > +++ gdb/parse.c 12 Sep 2011 15:25:46 -0000 > @@ -190,7 +190,7 @@ free_funcalls (void *ignore) > } > } > > -/* This page contains the functions for adding data to the struct expression > +/* This page contains the functions for adding data to the struct expression This is [obv]ious category, no need for an approval. > being constructed. */ > > /* Add one element to the end of the expression. */ > @@ -199,7 +199,7 @@ free_funcalls (void *ignore) > a register through here. */ > > void > -write_exp_elt (union exp_element expelt) > +write_exp_elt (union exp_element *expelt) This should be `const union exp_element *expelt' then. The patch from you does not compile: parse.c:202:1: error: conflicting types for ‘write_exp_elt’ parser-defs.h:134:13: note: previous declaration of ‘write_exp_elt’ was here In fact the parser-defs.h declaration should be removed and then write_exp_elt should be made static. But for the normal GDB production code -O2 -m64 this change has exactly the same code length; `union exp_element' by value is 16 bytes, therefore 2 registers but it saves handling the pointer indirection. AFAIK you do not have the copyright assignment but this change should not need the copyright assignment. As the source is longer and on -m64 it produces the same code I am not sure this patch is an advantage; but there are cases where it brings more optimal code (such as for -m32) so OK. > --- sim/ppc/dp-bit.c 1 Jan 2011 15:34:04 -0000 1.7 > +++ sim/ppc/dp-bit.c 12 Sep 2011 15:25:49 -0000 > @@ -408,7 +408,7 @@ pack_d ( fp_number_type * src) > } > > static void > -unpack_d (FLO_union_type * src, fp_number_type * dst) > +unpack_d (FLO_union_type *src, fp_number_type *dst) > { > fractype fraction = src->bits.fraction; > This is OK but unrelated to the patch above, this qualifies as [obv]ious patch. But the entry should be for sim/ppc/ChangeLog (and sure without the sim/ppc/ prefix). Thanks, Jan