Re: Some code-cleanup

[Abhijit Halder <abhijit.k.halder@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4759 bytes --]

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 #2: ChangeLog --]
[-- Type: application/octet-stream, Size: 503 bytes --]

2011-09-13  Abhijit Halder  <abhijit.k.halder@gmail.com>

	Code cleanup.
	* parse.c (write_exp_elt): Change argument to pass a pointer of union
	`exp_element' instead of an element of the same.
	(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' function call.
	* parser-defs.h (write_exp_elt): Change prototype.

[-- Attachment #3: code-cleanup.patch --]
[-- Type: text/x-patch, Size: 4407 bytes --]

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)
     }
 }
 \f
-/* 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
 }
 \f
 /* 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);