On 1/10/20 3:50 AM, Greg Kurz wrote:
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 :)
Or conversely play the game of which compilers will warn about an
atypical construct.
The MACHINE() macro is not a simple cast, it does object introspection
with OBJECT_CHECK(), thus is not free. Since
Sure, I understand the motivation in avoiding an unneeded call
to calling object_dynamic_cast_assert().
object_dynamic_cast_assert() argument is not const, I'm not sure the
compiler can remove the call.
Not remove the call, but delay it to the branch that uses it,
ie. parameter == RTAS_SYSPARM_SPLPAR_CHARACTERISTICS.
Richard, Eric, do you know?
Signed-off-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>
---
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;
Declaring an initializer inside a switch statement can trigger warnings
under some compilation scenarios (particularly if the variable is
referenced outside of the scope where it was introduced). But here, you
are using 'case label: { ...' to create a scope, which presumably ends
before the next case label, and is thus not going to trigger compiler
warnings.
An alternative is indeed leaving the declaration up front but deferring
the (possibly expensive) initializer to the case label that needs it:
MachineState *ms;
switch (parameter) {
case ...:
ms = MACHINE(spapr);
and done that way, you might not even need the extra {} behind the case
label (I didn't read the file to see if there is already some other
reason for having introduced that sub-scope).
As for whether compilers are smart enough to defer non-trivial
initialization to the one case label that uses the variable, I wouldn't
count on it. If the non-trivial initialization includes a function call
(which the MACHINE() macro does), it's much harder to prove whether that
function call has side effects that may be needed prior to the switch
statement. So limiting the scope of the initialization (whether by
dropping the declaration, or just deferring the call) DOES make sense.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
