Re: [Qemu-devel] [PATCH qom-cpu 1/4] cpu: Introduce CPUListState struct

[Eduardo Habkost]

On Tue, Dec 18, 2012 at 08:53:40AM +0100, Andreas Färber wrote:
> This generalizes {ARM,M68k,Alpha}CPUListState to avoid declaring it for
> each target.
> 
> Signed-off-by: Andreas Färber <address@hidden>
> ---
>  include/qemu/cpu.h   |   12 ++++++++++++
>  target-alpha/cpu.c   |    9 ++-------
>  target-arm/helper.c  |    9 ++-------
>  target-m68k/helper.c |    9 ++-------
>  4 Dateien geändert, 18 Zeilen hinzugefügt(+), 21 Zeilen entfernt(-)
> 
> diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
> index 61b7698..5fbb3f9 100644
> --- a/include/qemu/cpu.h
> +++ b/include/qemu/cpu.h
> @@ -21,6 +21,7 @@
>  #define QEMU_CPU_H
>  
>  #include "qemu/object.h"
> +#include "qemu-common.h"
>  #include "qemu-thread.h"

Please, don't add more "#include qemu-common.h" lines to header files.
This introduces yet another circular dependency:

qemu-common.h -> target-*/cpu.h -> target-*/cpu-qom.h -> qemu/cpu.h -> 
qemu-common.h

You could just reverse the order of patches 1/4 and 2/4, and include
"qemu-types.h" instead.

The rest of the patch is an obvious removal of duplicate code, that
would get a Reviewed-By line from me.

>  
>  /**
> @@ -80,6 +81,17 @@ struct CPUState {
>      /* TODO Move common fields from CPUArchState here. */
>  };
>  
> +/**
> + * CPUListState:
> + * @cpu_fprintf: Print function.
> + * @file: File to print to using @cpu_fprint.
> + *
> + * State commonly used for iterating over CPU models.
> + */
> +typedef struct CPUListState {
> +    fprintf_function cpu_fprintf;
> +    FILE *file;
> +} CPUListState;
>  
>  /**
>   * cpu_reset:
> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
> index d065085..915278f 100644
> --- a/target-alpha/cpu.c
> +++ b/target-alpha/cpu.c
> @@ -33,11 +33,6 @@ static void alpha_cpu_realize(Object *obj, Error **err)
>  #endif
>  }
>  
> -typedef struct AlphaCPUListState {
> -    fprintf_function cpu_fprintf;
> -    FILE *file;
> -} AlphaCPUListState;
> -
>  /* Sort alphabetically by type name. */
>  static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b)
>  {
> @@ -53,7 +48,7 @@ static gint alpha_cpu_list_compare(gconstpointer a, 
> gconstpointer b)
>  static void alpha_cpu_list_entry(gpointer data, gpointer user_data)
>  {
>      ObjectClass *oc = data;
> -    AlphaCPUListState *s = user_data;
> +    CPUListState *s = user_data;
>  
>      (*s->cpu_fprintf)(s->file, "  %s\n",
>                        object_class_get_name(oc));
> @@ -61,7 +56,7 @@ static void alpha_cpu_list_entry(gpointer data, gpointer 
> user_data)
>  
>  void alpha_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>  {
> -    AlphaCPUListState s = {
> +    CPUListState s = {
>          .file = f,
>          .cpu_fprintf = cpu_fprintf,
>      };
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index ab8b734..d2f2fb4 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1291,11 +1291,6 @@ ARMCPU *cpu_arm_init(const char *cpu_model)
>      return cpu;
>  }
>  
> -typedef struct ARMCPUListState {
> -    fprintf_function cpu_fprintf;
> -    FILE *file;
> -} ARMCPUListState;
> -
>  /* Sort alphabetically by type name, except for "any". */
>  static gint arm_cpu_list_compare(gconstpointer a, gconstpointer b)
>  {
> @@ -1317,7 +1312,7 @@ static gint arm_cpu_list_compare(gconstpointer a, 
> gconstpointer b)
>  static void arm_cpu_list_entry(gpointer data, gpointer user_data)
>  {
>      ObjectClass *oc = data;
> -    ARMCPUListState *s = user_data;
> +    CPUListState *s = user_data;
>  
>      (*s->cpu_fprintf)(s->file, "  %s\n",
>                        object_class_get_name(oc));
> @@ -1325,7 +1320,7 @@ static void arm_cpu_list_entry(gpointer data, gpointer 
> user_data)
>  
>  void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>  {
> -    ARMCPUListState s = {
> +    CPUListState s = {
>          .file = f,
>          .cpu_fprintf = cpu_fprintf,
>      };
> diff --git a/target-m68k/helper.c b/target-m68k/helper.c
> index a5d0100..875a71a 100644
> --- a/target-m68k/helper.c
> +++ b/target-m68k/helper.c
> @@ -25,11 +25,6 @@
>  
>  #define SIGNBIT (1u << 31)
>  
> -typedef struct M68kCPUListState {
> -    fprintf_function cpu_fprintf;
> -    FILE *file;
> -} M68kCPUListState;
> -
>  /* Sort alphabetically, except for "any". */
>  static gint m68k_cpu_list_compare(gconstpointer a, gconstpointer b)
>  {
> @@ -51,7 +46,7 @@ static gint m68k_cpu_list_compare(gconstpointer a, 
> gconstpointer b)
>  static void m68k_cpu_list_entry(gpointer data, gpointer user_data)
>  {
>      ObjectClass *c = data;
> -    M68kCPUListState *s = user_data;
> +    CPUListState *s = user_data;
>  
>      (*s->cpu_fprintf)(s->file, "%s\n",
>                        object_class_get_name(c));
> @@ -59,7 +54,7 @@ static void m68k_cpu_list_entry(gpointer data, gpointer 
> user_data)
>  
>  void m68k_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>  {
> -    M68kCPUListState s = {
> +    CPUListState s = {
>          .file = f,
>          .cpu_fprintf = cpu_fprintf,
>      };
> -- 
> 1.7.10.4
> 
> 

-- 
Eduardo