On Fri, 28 Oct 2022, Mark Cave-Ayland wrote:
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.
I think duplicating the same calculation for both initrd_base and
cmdline_base in the else branch is error prone so removing that is better
but I've sent a v6 with this patch removed so you can chose which one to
apply before the freeze.
Regards,
BALATON Zoltan
