This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
2011/9/13 Jan Kratochvil <jan.kratochvil@redhat.com>: > 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 ?<abhijit.k.halder@gmail.com> >> + >> + ? ? 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 > 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.
Attachment:
ChangeLog
Description: Binary data
Index: gdb/parse.c =================================================================== RCS file: /cvs/src/src/gdb/parse.c,v retrieving revision 1.110 diff -u -p -r1.110 parse.c --- gdb/parse.c 17 Jun 2011 20:24:22 -0000 1.110 +++ gdb/parse.c 13 Sep 2011 12:09:19 -0000 @@ -23,7 +23,7 @@ along with this program. If not, see <http://www.gnu.org/licenses/>. */ /* Parse an expression from text in a string, - and return the result as a struct expression pointer. + and return the result as a struct expression pointer. That structure contains arithmetic operations in reverse polish, with constants represented by operations that are followed by special data. See expression.h for the details of the format. @@ -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 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 (const union exp_element *expelt) { if (expout_ptr >= expout_size) { @@ -208,7 +208,7 @@ write_exp_elt (union exp_element expelt) xrealloc ((char *) expout, sizeof (struct expression) + EXP_ELEM_TO_BYTES (expout_size)); } - expout->elts[expout_ptr++] = expelt; + expout->elts[expout_ptr++] = *expelt; } void @@ -218,7 +218,7 @@ write_exp_elt_opcode (enum exp_opcode ex memset (&tmp, 0, sizeof (union exp_element)); tmp.opcode = expelt; - write_exp_elt (tmp); + write_exp_elt (&tmp); } void @@ -228,7 +228,7 @@ write_exp_elt_sym (struct symbol *expelt memset (&tmp, 0, sizeof (union exp_element)); tmp.symbol = expelt; - write_exp_elt (tmp); + write_exp_elt (&tmp); } void @@ -238,7 +238,7 @@ write_exp_elt_block (struct block *b) memset (&tmp, 0, sizeof (union exp_element)); tmp.block = b; - write_exp_elt (tmp); + write_exp_elt (&tmp); } void @@ -248,7 +248,7 @@ write_exp_elt_objfile (struct objfile *o memset (&tmp, 0, sizeof (union exp_element)); tmp.objfile = objfile; - write_exp_elt (tmp); + write_exp_elt (&tmp); } void @@ -258,7 +258,7 @@ write_exp_elt_longcst (LONGEST expelt) memset (&tmp, 0, sizeof (union exp_element)); tmp.longconst = expelt; - write_exp_elt (tmp); + write_exp_elt (&tmp); } void @@ -268,7 +268,7 @@ write_exp_elt_dblcst (DOUBLEST expelt) memset (&tmp, 0, sizeof (union exp_element)); tmp.doubleconst = expelt; - write_exp_elt (tmp); + write_exp_elt (&tmp); } void @@ -280,7 +280,7 @@ write_exp_elt_decfloatcst (gdb_byte expe for (index = 0; index < 16; index++) tmp.decfloatconst[index] = expelt[index]; - write_exp_elt (tmp); + write_exp_elt (&tmp); } void @@ -290,7 +290,7 @@ write_exp_elt_type (struct type *expelt) memset (&tmp, 0, sizeof (union exp_element)); tmp.type = expelt; - write_exp_elt (tmp); + write_exp_elt (&tmp); } void @@ -300,7 +300,7 @@ write_exp_elt_intern (struct internalvar memset (&tmp, 0, sizeof (union exp_element)); tmp.internalvar = expelt; - write_exp_elt (tmp); + write_exp_elt (&tmp); } /* Add a string constant to the end of the expression. @@ -1059,7 +1059,7 @@ prefixify_subexp (struct expression *ine } /* Read an expression from the string *STRINGPTR points to, - parse it, and return a pointer to a struct expression that we malloc. + parse it, and return a pointer to a struct expression that we malloc. Use block BLOCK as the lexical context for variable names; if BLOCK is zero, use the block of the selected stack frame. Meanwhile, advance *STRINGPTR to point after the expression, Index: gdb/parser-defs.h =================================================================== RCS file: /cvs/src/src/gdb/parser-defs.h,v retrieving revision 1.39 diff -u -p -r1.39 parser-defs.h --- gdb/parser-defs.h 10 Jan 2011 20:38:49 -0000 1.39 +++ gdb/parser-defs.h 13 Sep 2011 12:09:19 -0000 @@ -131,7 +131,7 @@ union type_stack_elt extern union type_stack_elt *type_stack; extern int type_stack_depth, type_stack_size; -extern void write_exp_elt (union exp_element); +extern void write_exp_elt (const union exp_element *); extern void write_exp_elt_opcode (enum exp_opcode);
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |