On 14/10/2022 16:25, BALATON Zoltan wrote:
On Fri, 14 Oct 2022, BALATON Zoltan wrote:
On Fri, 14 Oct 2022, Mark Cave-Ayland wrote:
On 03/10/2022 21:13, BALATON Zoltan wrote:
By slight reorganisation we can avoid an else branch and some code
duplication which makes it easier to follow the code.
Signed-off-by: BALATON Zoltan <[email protected]>
---
hw/ppc/mac_newworld.c | 6 +++---
hw/ppc/mac_oldworld.c | 7 +++----
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 6bc3bd19be..73b01e8c6d 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -194,9 +194,11 @@ static void ppc_core99_init(MachineState *machine)
machine->kernel_filename);
exit(1);
}
+ cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
+ KERNEL_GAP);
/* load initrd */
if (machine->initrd_filename) {
- initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
KERNEL_GAP);
+ initrd_base = cmdline_base;
initrd_size = load_image_targphys(machine->initrd_filename,
initrd_base,
machine->ram_size -
initrd_base);
@@ -206,8 +208,6 @@ static void ppc_core99_init(MachineState *machine)
exit(1);
}
cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
- } else {
- cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
KERNEL_GAP);
}
ppc_boot_device = 'm';
} else {
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index cb67e44081..b424729a39 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -168,10 +168,11 @@ static void ppc_heathrow_init(MachineState *machine)
machine->kernel_filename);
exit(1);
}
+ cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
+ KERNEL_GAP);
/* load initrd */
if (machine->initrd_filename) {
- initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
- KERNEL_GAP);
+ initrd_base = cmdline_base;
initrd_size = load_image_targphys(machine->initrd_filename,
initrd_base,
machine->ram_size -
initrd_base);
@@ -181,8 +182,6 @@ static void ppc_heathrow_init(MachineState *machine)
exit(1);
}
cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
- } else {
- cmdline_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
KERNEL_GAP);
}
ppc_boot_device = 'm';
} else {
Is there any particular reason why you would want to avoid the else branch? I
It avoids code duplication and to me it's easier to follow than an else branch.
With this patch cmdline_base calculation is clearly tied to kernel_base and
kernel_size if only kernel is used and initrd_base initrd_size when initrd is used.
With the else branch it's less obvious because it's set much later in the else
branch while initrd_base duplicates it above. So avoiding this duplication makes
the code easier to read and less prone to errors if the calculation is ever modified.
don't feel this patch is an improvement since by always setting cmdline_base to a
non-zero value, as a reviewer I then have to check all other uses of cmdline_base
in the file to ensure that this doesn't cause any issues.
There aren't that many uses that it's difficult to check and this only need to be
done once.
I much prefer the existing version since setting the values of cmdline_base and
initrd_base is very clearly scoped within the if statement.
What can I say, it's hard to argue with preferences but avoiding code duplication
is worth the effort reviewing this patch in my opinion.
Also compare the before and after this series:
https://github.com/patchew-project/qemu/blob/master/hw/ppc/mac_newworld.c
https://github.com/patchew-project/qemu/blob/9c1cd2828b3d3bd3a7068134a57ae9bb07f5a681/hw/ppc/mac_newworld.c
I think the result is much easier to follow without else brances as you can read it
from top to bottom instead of jumping around in else branches that is less clear and
also more lines. Also setting default value avoids any used uninitialised cases which
you would need to check if you set vars in if-else instead so unlike what you say
this does not introduce such cases but closes the possibility instead. So please take
the time to review it in exchange of the time I've put it in writing it.
I've revisited this looking at the links provided above, and after consideration my
opinion is that that having the more localised scoping of the variables is more
worthwhile (i.e. the compiler can better catch errors) rather than eliminating the
duplication for a couple of lines. Please drop this patch before posting the next
version of the series.
ATB,
Mark.
