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 <bala...@eik.bme.hu> --- 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? IIt 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.chttps://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
