Re: Some code-cleanup

[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)
>      }
>  }
>  \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

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