[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Philippe Mathieu-Daudé
Subject: Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case
Date: Fri, 10 Jan 2020 10:34:07 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2

On 1/9/20 6:43 PM, Greg Kurz wrote:
On Thu,  9 Jan 2020 16:21:22 +0100
Philippe Mathieu-Daudé <address@hidden> 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.

I don't want to outsmart compilers :)

The MACHINE() macro is not a simple cast, it does object introspection with OBJECT_CHECK(), thus is not free. Since object_dynamic_cast_assert() argument is not const, I'm not sure the compiler can remove the call.

Richard, Eric, do you know?

Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
  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) {
+        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 ","
                                           current_machine->ram_size / MiB,

OK, good idea.

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 ","

reply via email to

[Prev in Thread] Current Thread [Next in Thread]