From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25083 invoked by alias); 13 Sep 2011 12:20:55 -0000 Received: (qmail 25059 invoked by uid 22791); 13 Sep 2011 12:20:51 -0000 X-SWARE-Spam-Status: No, hits=-2.6 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,TW_BJ X-Spam-Check-By: sourceware.org Received: from mail-ww0-f41.google.com (HELO mail-ww0-f41.google.com) (74.125.82.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 13 Sep 2011 12:20:17 +0000 Received: by wwf10 with SMTP id 10so2704886wwf.0 for ; Tue, 13 Sep 2011 05:20:15 -0700 (PDT) MIME-Version: 1.0 Received: by 10.216.169.74 with SMTP id m52mr1213698wel.33.1315916415654; Tue, 13 Sep 2011 05:20:15 -0700 (PDT) Received: by 10.216.159.205 with HTTP; Tue, 13 Sep 2011 05:20:15 -0700 (PDT) In-Reply-To: <20110913092440.GA12661@host1.jankratochvil.net> References: <201109121623.04292.pedro@codesourcery.com> <20110913092440.GA12661@host1.jankratochvil.net> Date: Tue, 13 Sep 2011 12:55:00 -0000 Message-ID: Subject: Re: Some code-cleanup From: Abhijit Halder To: Jan Kratochvil Cc: Pedro Alves , gdb-patches@sourceware.org Content-Type: multipart/mixed; boundary=0016367fb52f825abb04acd1aa5e 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/msg00209.txt.bz2 --0016367fb52f825abb04acd1aa5e Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Content-length: 5025 2011/9/13 Jan Kratochvil : > On Mon, 12 Sep 2011 17:30:06 +0200, Abhijit Halder wrote: >> Index: gdb/ChangeLog >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> RCS file: /cvs/src/src/gdb/ChangeLog,v >> retrieving revision 1.13320 >> diff -u -p -r1.13320 ChangeLog >> --- gdb/ChangeLog =A0 =A0 11 Sep 2011 09:54:16 -0000 =A0 =A0 =A01.13320 >> +++ gdb/ChangeLog =A0 =A0 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 =A0Abhijit Halder =A0 >> + >> + =A0 =A0 Code cleanup. >> + =A0 =A0 * gdb/parse.c (write_exp_elt): Change the argument to pass a p= ointer > > There should be only "parse.c", as it is in gdb/ChangeLog. > >> + =A0 =A0 of union exp_element instead of an element of the same. > > >> + =A0 =A0 * (write_exp_elt_opcode): Change argument of write_exp_elt cal= l. >> + =A0 =A0 * (write_exp_elt_sym): Change argument of write_exp_elt call. >> + =A0 =A0 * (write_exp_elt_block): Change argument of write_exp_elt call. >> + =A0 =A0 * (write_exp_elt_objfile): Change argument of write_exp_elt ca= ll. >> + =A0 =A0 * (write_exp_elt_longcst): Change argument of write_exp_elt ca= ll. >> + =A0 =A0 * (write_exp_elt_dblcst): Change argument of write_exp_elt cal= l. >> + =A0 =A0 * (write_exp_elt_decfloatcst): Change argument of write_exp_el= t call. >> + =A0 =A0 * (write_exp_elt_type): Change argument of write_exp_elt call. >> + =A0 =A0 * (write_exp_elt_intern): Change argument of write_exp_elt cal= l. > > In these lines there should not be `* ' as it is not a new file. =A0And t= he > entries for functions in the same file should be merged. =A0See examples = in the > GNU Coding Style document: > =A0 =A0 =A0 =A0(write_exp_elt_opcode, write_exp_elt_sym, write_exp_elt_bl= ock) > =A0 =A0 =A0 =A0(write_exp_elt_objfile, write_exp_elt_longcst, write_exp_e= lt_dblcst) > =A0 =A0 =A0 =A0(write_exp_elt_decfloatcst, write_exp_elt_type, write_exp_= elt_intern): > =A0 =A0 =A0 =A0Change argument of write_exp_elt call. > > >> + =A0 =A0 * src/sim/ppc/dp-bit.c (unpack_d): Change the formatting. > > Inappropriate here, see at the bottom. > > >> --- gdb/parse.c =A0 =A0 =A0 17 Jun 2011 20:24:22 -0000 =A0 =A0 =A01.110 >> +++ gdb/parse.c =A0 =A0 =A0 12 Sep 2011 15:25:46 -0000 >> @@ -190,7 +190,7 @@ free_funcalls (void *ignore) >> =A0 =A0 =A0} >> =A0} >> >> -/* This page contains the functions for adding data to the =A0struct ex= pression >> +/* This page contains the functions for adding data to the struct expre= ssion > > This is [obv]ious category, no need for an approval. > > >> =A0 =A0 being constructed. =A0*/ >> >> =A0/* Add one element to the end of the expression. =A0*/ >> @@ -199,7 +199,7 @@ free_funcalls (void *ignore) >> =A0 =A0 a register through here. =A0*/ >> >> =A0void >> -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: > =A0 =A0 =A0 =A0parse.c:202:1: error: conflicting types for =91write_exp_e= lt=92 > =A0 =A0 =A0 =A0parser-defs.h:134:13: note: previous declaration of =91wri= te_exp_elt=92 was here > > In fact the parser-defs.h declaration should be removed and then write_ex= p_elt > should be made static. > > But for the normal GDB production code -O2 -m64 this change has exactly t= he > 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. =A0As the source is longer and on -m64 it produ= ces the > same code I am not sure this patch is an advantage; but there are cases w= here > it brings more optimal code (such as for -m32) so OK. > > >> --- sim/ppc/dp-bit.c =A01 Jan 2011 15:34:04 -0000 =A0 =A0 =A0 1.7 >> +++ sim/ppc/dp-bit.c =A012 Sep 2011 15:25:49 -0000 >> @@ -408,7 +408,7 @@ pack_d ( fp_number_type * =A0src) >> =A0} >> >> =A0static void >> -unpack_d (FLO_union_type * src, fp_number_type * dst) >> +unpack_d (FLO_union_type *src, fp_number_type *dst) >> =A0{ >> =A0 =A0fractype fraction =3D 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/p= pc/ > prefix). > > > Thanks, > Jan > Not sure whether we should check in this change as Jan mentioned that with -m64 the union and its pointer will have same word length, but using pointer will cause the pointer indirection, which has its own overhead. I am re-submitting the patch with suggested corrections. Please review the = same. --0016367fb52f825abb04acd1aa5e Content-Type: application/octet-stream; name=ChangeLog Content-Disposition: attachment; filename=ChangeLog Content-Transfer-Encoding: base64 X-Attachment-Id: f_gsiuh3md0 Content-length: 684 MjAxMS0wOS0xMyAgQWJoaWppdCBIYWxkZXIgIDxhYmhpaml0LmsuaGFsZGVy QGdtYWlsLmNvbT4KCglDb2RlIGNsZWFudXAuCgkqIHBhcnNlLmMgKHdyaXRl X2V4cF9lbHQpOiBDaGFuZ2UgYXJndW1lbnQgdG8gcGFzcyBhIHBvaW50ZXIg b2YgdW5pb24KCWBleHBfZWxlbWVudCcgaW5zdGVhZCBvZiBhbiBlbGVtZW50 IG9mIHRoZSBzYW1lLgoJKHdyaXRlX2V4cF9lbHRfb3Bjb2RlLCB3cml0ZV9l eHBfZWx0X3N5bSwgd3JpdGVfZXhwX2VsdF9ibG9jaykKCSh3cml0ZV9leHBf ZWx0X29iamZpbGUsIHdyaXRlX2V4cF9lbHRfbG9uZ2NzdCwgd3JpdGVfZXhw X2VsdF9kYmxjc3QpCgkod3JpdGVfZXhwX2VsdF9kZWNmbG9hdGNzdCwgd3Jp dGVfZXhwX2VsdF90eXBlLCB3cml0ZV9leHBfZWx0X2ludGVybik6CglDaGFu Z2UgYXJndW1lbnQgb2YgYHdyaXRlX2V4cF9lbHQnIGZ1bmN0aW9uIGNhbGwu CgkqIHBhcnNlci1kZWZzLmggKHdyaXRlX2V4cF9lbHQpOiBDaGFuZ2UgcHJv dG90eXBlLgo= --0016367fb52f825abb04acd1aa5e Content-Type: text/x-patch; charset=US-ASCII; name="code-cleanup.patch" Content-Disposition: attachment; filename="code-cleanup.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_gsiuhpv41 Content-length: 5974 SW5kZXg6IGdkYi9wYXJzZS5jCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KUkNT IGZpbGU6IC9jdnMvc3JjL3NyYy9nZGIvcGFyc2UuYyx2CnJldHJpZXZpbmcg cmV2aXNpb24gMS4xMTAKZGlmZiAtdSAtcCAtcjEuMTEwIHBhcnNlLmMKLS0t IGdkYi9wYXJzZS5jCTE3IEp1biAyMDExIDIwOjI0OjIyIC0wMDAwCTEuMTEw CisrKyBnZGIvcGFyc2UuYwkxMyBTZXAgMjAxMSAxMjowOToxOSAtMDAwMApA QCAtMjMsNyArMjMsNyBAQAogICAgYWxvbmcgd2l0aCB0aGlzIHByb2dyYW0u ICBJZiBub3QsIHNlZSA8aHR0cDovL3d3dy5nbnUub3JnL2xpY2Vuc2VzLz4u ICAqLwogCiAvKiBQYXJzZSBhbiBleHByZXNzaW9uIGZyb20gdGV4dCBpbiBh IHN0cmluZywKLSAgIGFuZCByZXR1cm4gdGhlIHJlc3VsdCBhcyBhICBzdHJ1 Y3QgZXhwcmVzc2lvbiAgcG9pbnRlci4KKyAgIGFuZCByZXR1cm4gdGhlIHJl c3VsdCBhcyBhIHN0cnVjdCBleHByZXNzaW9uIHBvaW50ZXIuCiAgICBUaGF0 IHN0cnVjdHVyZSBjb250YWlucyBhcml0aG1ldGljIG9wZXJhdGlvbnMgaW4g cmV2ZXJzZSBwb2xpc2gsCiAgICB3aXRoIGNvbnN0YW50cyByZXByZXNlbnRl ZCBieSBvcGVyYXRpb25zIHRoYXQgYXJlIGZvbGxvd2VkIGJ5IHNwZWNpYWwg ZGF0YS4KICAgIFNlZSBleHByZXNzaW9uLmggZm9yIHRoZSBkZXRhaWxzIG9m IHRoZSBmb3JtYXQuCkBAIC0xOTAsNyArMTkwLDcgQEAgZnJlZV9mdW5jYWxs cyAodm9pZCAqaWdub3JlKQogICAgIH0KIH0KIAwKLS8qIFRoaXMgcGFnZSBj b250YWlucyB0aGUgZnVuY3Rpb25zIGZvciBhZGRpbmcgZGF0YSB0byB0aGUg IHN0cnVjdCBleHByZXNzaW9uCisvKiBUaGlzIHBhZ2UgY29udGFpbnMgdGhl IGZ1bmN0aW9ucyBmb3IgYWRkaW5nIGRhdGEgdG8gdGhlIHN0cnVjdCBleHBy ZXNzaW9uCiAgICBiZWluZyBjb25zdHJ1Y3RlZC4gICovCiAKIC8qIEFkZCBv bmUgZWxlbWVudCB0byB0aGUgZW5kIG9mIHRoZSBleHByZXNzaW9uLiAgKi8K QEAgLTE5OSw3ICsxOTksNyBAQCBmcmVlX2Z1bmNhbGxzICh2b2lkICppZ25v cmUpCiAgICBhIHJlZ2lzdGVyIHRocm91Z2ggaGVyZS4gICovCiAKIHZvaWQK LXdyaXRlX2V4cF9lbHQgKHVuaW9uIGV4cF9lbGVtZW50IGV4cGVsdCkKK3dy aXRlX2V4cF9lbHQgKGNvbnN0IHVuaW9uIGV4cF9lbGVtZW50ICpleHBlbHQp CiB7CiAgIGlmIChleHBvdXRfcHRyID49IGV4cG91dF9zaXplKQogICAgIHsK QEAgLTIwOCw3ICsyMDgsNyBAQCB3cml0ZV9leHBfZWx0ICh1bmlvbiBleHBf ZWxlbWVudCBleHBlbHQpCiAJeHJlYWxsb2MgKChjaGFyICopIGV4cG91dCwg c2l6ZW9mIChzdHJ1Y3QgZXhwcmVzc2lvbikKIAkJICArIEVYUF9FTEVNX1RP X0JZVEVTIChleHBvdXRfc2l6ZSkpOwogICAgIH0KLSAgZXhwb3V0LT5lbHRz W2V4cG91dF9wdHIrK10gPSBleHBlbHQ7CisgIGV4cG91dC0+ZWx0c1tleHBv dXRfcHRyKytdID0gKmV4cGVsdDsKIH0KIAogdm9pZApAQCAtMjE4LDcgKzIx OCw3IEBAIHdyaXRlX2V4cF9lbHRfb3Bjb2RlIChlbnVtIGV4cF9vcGNvZGUg ZXgKIAogICBtZW1zZXQgKCZ0bXAsIDAsIHNpemVvZiAodW5pb24gZXhwX2Vs ZW1lbnQpKTsKICAgdG1wLm9wY29kZSA9IGV4cGVsdDsKLSAgd3JpdGVfZXhw X2VsdCAodG1wKTsKKyAgd3JpdGVfZXhwX2VsdCAoJnRtcCk7CiB9CiAKIHZv aWQKQEAgLTIyOCw3ICsyMjgsNyBAQCB3cml0ZV9leHBfZWx0X3N5bSAoc3Ry dWN0IHN5bWJvbCAqZXhwZWx0CiAKICAgbWVtc2V0ICgmdG1wLCAwLCBzaXpl b2YgKHVuaW9uIGV4cF9lbGVtZW50KSk7CiAgIHRtcC5zeW1ib2wgPSBleHBl bHQ7Ci0gIHdyaXRlX2V4cF9lbHQgKHRtcCk7CisgIHdyaXRlX2V4cF9lbHQg KCZ0bXApOwogfQogCiB2b2lkCkBAIC0yMzgsNyArMjM4LDcgQEAgd3JpdGVf ZXhwX2VsdF9ibG9jayAoc3RydWN0IGJsb2NrICpiKQogCiAgIG1lbXNldCAo JnRtcCwgMCwgc2l6ZW9mICh1bmlvbiBleHBfZWxlbWVudCkpOwogICB0bXAu YmxvY2sgPSBiOwotICB3cml0ZV9leHBfZWx0ICh0bXApOworICB3cml0ZV9l eHBfZWx0ICgmdG1wKTsKIH0KIAogdm9pZApAQCAtMjQ4LDcgKzI0OCw3IEBA IHdyaXRlX2V4cF9lbHRfb2JqZmlsZSAoc3RydWN0IG9iamZpbGUgKm8KIAog ICBtZW1zZXQgKCZ0bXAsIDAsIHNpemVvZiAodW5pb24gZXhwX2VsZW1lbnQp KTsKICAgdG1wLm9iamZpbGUgPSBvYmpmaWxlOwotICB3cml0ZV9leHBfZWx0 ICh0bXApOworICB3cml0ZV9leHBfZWx0ICgmdG1wKTsKIH0KIAogdm9pZApA QCAtMjU4LDcgKzI1OCw3IEBAIHdyaXRlX2V4cF9lbHRfbG9uZ2NzdCAoTE9O R0VTVCBleHBlbHQpCiAKICAgbWVtc2V0ICgmdG1wLCAwLCBzaXplb2YgKHVu aW9uIGV4cF9lbGVtZW50KSk7CiAgIHRtcC5sb25nY29uc3QgPSBleHBlbHQ7 Ci0gIHdyaXRlX2V4cF9lbHQgKHRtcCk7CisgIHdyaXRlX2V4cF9lbHQgKCZ0 bXApOwogfQogCiB2b2lkCkBAIC0yNjgsNyArMjY4LDcgQEAgd3JpdGVfZXhw X2VsdF9kYmxjc3QgKERPVUJMRVNUIGV4cGVsdCkKIAogICBtZW1zZXQgKCZ0 bXAsIDAsIHNpemVvZiAodW5pb24gZXhwX2VsZW1lbnQpKTsKICAgdG1wLmRv dWJsZWNvbnN0ID0gZXhwZWx0OwotICB3cml0ZV9leHBfZWx0ICh0bXApOwor ICB3cml0ZV9leHBfZWx0ICgmdG1wKTsKIH0KIAogdm9pZApAQCAtMjgwLDcg KzI4MCw3IEBAIHdyaXRlX2V4cF9lbHRfZGVjZmxvYXRjc3QgKGdkYl9ieXRl IGV4cGUKICAgZm9yIChpbmRleCA9IDA7IGluZGV4IDwgMTY7IGluZGV4Kysp CiAgICAgdG1wLmRlY2Zsb2F0Y29uc3RbaW5kZXhdID0gZXhwZWx0W2luZGV4 XTsKIAotICB3cml0ZV9leHBfZWx0ICh0bXApOworICB3cml0ZV9leHBfZWx0 ICgmdG1wKTsKIH0KIAogdm9pZApAQCAtMjkwLDcgKzI5MCw3IEBAIHdyaXRl X2V4cF9lbHRfdHlwZSAoc3RydWN0IHR5cGUgKmV4cGVsdCkKIAogICBtZW1z ZXQgKCZ0bXAsIDAsIHNpemVvZiAodW5pb24gZXhwX2VsZW1lbnQpKTsKICAg dG1wLnR5cGUgPSBleHBlbHQ7Ci0gIHdyaXRlX2V4cF9lbHQgKHRtcCk7Cisg IHdyaXRlX2V4cF9lbHQgKCZ0bXApOwogfQogCiB2b2lkCkBAIC0zMDAsNyAr MzAwLDcgQEAgd3JpdGVfZXhwX2VsdF9pbnRlcm4gKHN0cnVjdCBpbnRlcm5h bHZhcgogCiAgIG1lbXNldCAoJnRtcCwgMCwgc2l6ZW9mICh1bmlvbiBleHBf ZWxlbWVudCkpOwogICB0bXAuaW50ZXJuYWx2YXIgPSBleHBlbHQ7Ci0gIHdy aXRlX2V4cF9lbHQgKHRtcCk7CisgIHdyaXRlX2V4cF9lbHQgKCZ0bXApOwog fQogCiAvKiBBZGQgYSBzdHJpbmcgY29uc3RhbnQgdG8gdGhlIGVuZCBvZiB0 aGUgZXhwcmVzc2lvbi4KQEAgLTEwNTksNyArMTA1OSw3IEBAIHByZWZpeGlm eV9zdWJleHAgKHN0cnVjdCBleHByZXNzaW9uICppbmUKIH0KIAwKIC8qIFJl YWQgYW4gZXhwcmVzc2lvbiBmcm9tIHRoZSBzdHJpbmcgKlNUUklOR1BUUiBw b2ludHMgdG8sCi0gICBwYXJzZSBpdCwgYW5kIHJldHVybiBhIHBvaW50ZXIg dG8gYSAgc3RydWN0IGV4cHJlc3Npb24gIHRoYXQgd2UgbWFsbG9jLgorICAg cGFyc2UgaXQsIGFuZCByZXR1cm4gYSBwb2ludGVyIHRvIGEgc3RydWN0IGV4 cHJlc3Npb24gdGhhdCB3ZSBtYWxsb2MuCiAgICBVc2UgYmxvY2sgQkxPQ0sg YXMgdGhlIGxleGljYWwgY29udGV4dCBmb3IgdmFyaWFibGUgbmFtZXM7CiAg ICBpZiBCTE9DSyBpcyB6ZXJvLCB1c2UgdGhlIGJsb2NrIG9mIHRoZSBzZWxl Y3RlZCBzdGFjayBmcmFtZS4KICAgIE1lYW53aGlsZSwgYWR2YW5jZSAqU1RS SU5HUFRSIHRvIHBvaW50IGFmdGVyIHRoZSBleHByZXNzaW9uLApJbmRleDog Z2RiL3BhcnNlci1kZWZzLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQpSQ1Mg ZmlsZTogL2N2cy9zcmMvc3JjL2dkYi9wYXJzZXItZGVmcy5oLHYKcmV0cmll dmluZyByZXZpc2lvbiAxLjM5CmRpZmYgLXUgLXAgLXIxLjM5IHBhcnNlci1k ZWZzLmgKLS0tIGdkYi9wYXJzZXItZGVmcy5oCTEwIEphbiAyMDExIDIwOjM4 OjQ5IC0wMDAwCTEuMzkKKysrIGdkYi9wYXJzZXItZGVmcy5oCTEzIFNlcCAy MDExIDEyOjA5OjE5IC0wMDAwCkBAIC0xMzEsNyArMTMxLDcgQEAgdW5pb24g dHlwZV9zdGFja19lbHQKIGV4dGVybiB1bmlvbiB0eXBlX3N0YWNrX2VsdCAq dHlwZV9zdGFjazsKIGV4dGVybiBpbnQgdHlwZV9zdGFja19kZXB0aCwgdHlw ZV9zdGFja19zaXplOwogCi1leHRlcm4gdm9pZCB3cml0ZV9leHBfZWx0ICh1 bmlvbiBleHBfZWxlbWVudCk7CitleHRlcm4gdm9pZCB3cml0ZV9leHBfZWx0 IChjb25zdCB1bmlvbiBleHBfZWxlbWVudCAqKTsKIAogZXh0ZXJuIHZvaWQg d3JpdGVfZXhwX2VsdF9vcGNvZGUgKGVudW0gZXhwX29wY29kZSk7CiAK --0016367fb52f825abb04acd1aa5e--