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

[Philippe Mathieu-Daudé]

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) {
> >       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);

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 ","
> >                                             "DesProcs=%d,"