Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case

From: Greg Kurz <groug@kaod.org>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: qemu-devel@nongnu.org, Peter Maydell <peter.maydell@linaro.org>,
	Eduardo Habkost <ehabkost@redhat.com>,
	kvm@vger.kernel.org, Juan Quintela <quintela@redhat.com>,
	qemu-ppc@nongnu.org, Marcelo Tosatti <mtosatti@redhat.com>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	qemu-arm@nongnu.org, Alistair Francis <alistair.francis@wdc.com>,
	Marcel Apfelbaum <marcel.apfelbaum@gmail.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	David Gibson <david@gibson.dropbear.id.au>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case
Date: Thu, 9 Jan 2020 18:43:49 +0100	[thread overview]
Message-ID: <20200109184349.1aefa074@bahia.lan> (raw)
In-Reply-To: <20200109152133.23649-5-philmd@redhat.com>

On Thu,  9 Jan 2020 16:21:22 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> We only access these variables in RTAS_SYSPARM_SPLPAR_CHARACTERISTICS
> case, restrict their scope to avoid unnecessary initialization.
> 

I guess a decent compiler can be smart enough detect that the initialization
isn't needed outside of the RTAS_SYSPARM_SPLPAR_CHARACTERISTICS branch...
Anyway, reducing scope isn't bad. The only hitch I could see is that some
people do prefer to have all variables declared upfront, but there's a nested
param_val variable already so I guess it's okay.

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/ppc/spapr_rtas.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 6f06e9d7fe..7237e5ebf2 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -267,8 +267,6 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>                                            uint32_t nret, target_ulong rets)
>  {
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> -    MachineState *ms = MACHINE(spapr);
> -    unsigned int max_cpus = ms->smp.max_cpus;
>      target_ulong parameter = rtas_ld(args, 0);
>      target_ulong buffer = rtas_ld(args, 1);
>      target_ulong length = rtas_ld(args, 2);
> @@ -276,6 +274,8 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>  
>      switch (parameter) {
>      case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: {
> +        MachineState *ms = MACHINE(spapr);
> +        unsigned int max_cpus = ms->smp.max_cpus;

The max_cpus variable used to be a global. Now that it got moved
below ms->smp, I'm not sure it's worth keeping it IMHO. What about
dropping it completely and do:

        char *param_val = g_strdup_printf("MaxEntCap=%d,"
                                          "DesMem=%" PRIu64 ","
                                          "DesProcs=%d,"
                                          "MaxPlatProcs=%d",
                                          ms->smp.max_cpus,
                                          current_machine->ram_size / MiB,
                                          ms->smp.cpus,
                                          ms->smp.max_cpus);

And maybe insert an empty line between the declaration of param_val
and the code for a better readability ?

>          char *param_val = g_strdup_printf("MaxEntCap=%d,"
>                                            "DesMem=%" PRIu64 ","
>                                            "DesProcs=%d,"