Re: [U-Boot] [PATCH] treewide: compress lines for immediate return
Hi Stefano, 2016-09-06 18:25 GMT+09:00 Stefano Babic: > > Nothing against, but it looks to me just a different and allowed coding > style - where is the advantages of this ? > This had already been superseded. Please check v3, where I mostly transformed simple wrapper functions only. -- Best Regards Masahiro Yamada ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] treewide: compress lines for immediate return
Hi Stephen, 2016-09-07 1:09 GMT+09:00 Stephen Warren: >>> All child function calls are structured the same, so someone reading the >>> code will always see the same structure irrespective of where in a >>> function >>> a child function is called. This gives uniformity. This also yields a few >>> maintenance advantages below, and helps keep all code uniform even if any >>> of >>> the maintenance operations below have been applied to some functions and >>> aren't needed in others. >> >> >> >> Did you think I ran a semantic patch with Coccinelle >> and then sent it blindly? >> >> No, this patch passed my eyes' check, at least. >> >> Please notice this patch did not transform >> the following function in arch/arm/cpu/armv7/am33xx/clk_synthesizer.c > > ... > > The patch description clearly stated that the patch was purely the result of > applying the Coccinelle script. If there were exceptions or other edits, > they should have been explicitly mentioned too. Right. The git-log implied a semantic patch, but I did not mention that I sent the output of Coccinelle as is. Actually, I cherry-picked reasonable hunks. Coccinelle may sometimes do false positive jobs (or undesirable output like this case), so I think "Coccinelle + checking by eyes" is a good practice. I dropped a semantic patch snippet from v3 git-log. >> If we start to consider things that may happen or may not happen, >> we end up with adding redundancy all the time. >> >> Are you positive or negative for the following hunk? >> >>> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c >>> b/drivers/mtd/nand/fsl_elbc_nand.c >>> index f621f14..b27a6af 100644 >>> --- a/drivers/mtd/nand/fsl_elbc_nand.c >>> +++ b/drivers/mtd/nand/fsl_elbc_nand.c >>> @@ -788,11 +788,7 @@ static int fsl_elbc_chip_init(int devnum, u8 *addr) >>> if (ret) >>> return ret; >>> >>> - ret = nand_register(devnum, mtd); >>> - if (ret) >>> - return ret; >>> - >>> - return 0; >>> + return nand_register(devnum, mtd); >>> } > > > I'd probably tend not to do that particular conversion, for consistency with > the immediately preceding nand_scan_tail() case. Still, this one isn't such > an obvious call so I wouldn't feel particularly strongly about it, > especially as it isn't a driver I work on. > >> I think probe/init function can return a value >> of register function directly, from my best common sense. >> >> This change will lose 2) >> in case fsl_elbc_chip_init() fails to do nand_register, though. OK. I dropped those changes in v2. (I still personally believe "return *_register();" is good coding style, though. This might be a matter of preference...) In v3, I only fixed drivers/video/vidconsole-uclass.c because I thought it is simple enough. http://patchwork.ozlabs.org/patch/666560/ and it was acked by Anatolij. -- Best Regards Masahiro Yamada ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] treewide: compress lines for immediate return
On 09/03/2016 05:38 PM, Masahiro Yamada wrote: 2016-09-03 2:15 GMT+09:00 Stephen Warren: On 09/02/2016 04:36 AM, Masahiro Yamada wrote: -ret = expression; -if (ret) -return ret; -return 0; +return expression; I disagree with this change if applied blindly; I think both coding styles have their merit depending on the semantic context. In the case of a simple wrapper function that just calls another with different arguments, directly returning the result from the called function make sense. For example: int device_bind(struct udevice *parent, const struct driver *drv, const char *name, void *platdata, int of_offset, struct udevice **devp) { return device_bind_common(parent, drv, name, platdata, 0, of_offset, 0, devp); } However, where the top-level function is more complex, especially where it calls multiple functions in its body, I think it's best to use the exact same style to call all functions in the top-level body, and just "return 0" separately at the end. For example: static int tegra186_bpmp_bind(struct udevice *dev) { int ret; struct udevice *child; debug("%s(dev=%p)\n", __func__, dev); ret = device_bind_driver_to_node(dev, "tegra186_clk", "tegra186_clk", dev->of_offset, ); if (ret) return ret; ret = device_bind_driver_to_node(dev, "tegra186_reset", "tegra186_reset", dev->of_offset, ); if (ret) return ret; ret = device_bind_driver_to_node(dev, "tegra186_power_domain", "tegra186_power_domain", dev->of_offset, ); if (ret) return ret; ret = dm_scan_fdt_dev(dev); if (ret) return ret; return 0; } All child function calls are structured the same, so someone reading the code will always see the same structure irrespective of where in a function a child function is called. This gives uniformity. This also yields a few maintenance advantages below, and helps keep all code uniform even if any of the maintenance operations below have been applied to some functions and aren't needed in others. Did you think I ran a semantic patch with Coccinelle and then sent it blindly? No, this patch passed my eyes' check, at least. Please notice this patch did not transform the following function in arch/arm/cpu/armv7/am33xx/clk_synthesizer.c ... The patch description clearly stated that the patch was purely the result of applying the Coccinelle script. If there were exceptions or other edits, they should have been explicitly mentioned too. 1) If tegra186_bpmp_bind() were modified to call an additional function in its body, the diff for that addition would look identical no matter whether the new call was added at the start/middle/end of the body. We wouldn't ever have to do something like: - return dm_scan_fdt_dev(dev); + ret = dm_scan_fdt_dev(dev); + if (ret) + return ret; ... which is an edit to a piece of code that's unrelated to the code being added, and thus makes the patch more confusing. 2) There's always an obvious place to put any debug()/error() invocations, or translate return values; inside the if (ret) body. There's only one way for the code to look so it doesn't change based on what exactly we do with the return value. 3) If we add the need for cleanup logic in the failure case, we can just blindly change "return ret" to "goto fail" without re-structuring code. I am not sure if 2) and 3) are realistic. Well, they're both based on my having seen those exact things happen... If we start to consider things that may happen or may not happen, we end up with adding redundancy all the time. Are you positive or negative for the following hunk? diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c index f621f14..b27a6af 100644 --- a/drivers/mtd/nand/fsl_elbc_nand.c +++ b/drivers/mtd/nand/fsl_elbc_nand.c @@ -788,11 +788,7 @@ static int fsl_elbc_chip_init(int devnum, u8 *addr) if (ret) return ret; - ret = nand_register(devnum, mtd); - if (ret) - return ret; - - return 0; + return nand_register(devnum, mtd); } I'd probably tend not to do that particular conversion, for consistency with the immediately preceding nand_scan_tail() case. Still, this one isn't such an obvious call so I wouldn't feel particularly strongly about it, especially as it isn't a driver I work on. I think probe/init function can return a value of register function directly, from my best common sense. This change will lose 2) in case fsl_elbc_chip_init() fails to do nand_register, though.
Re: [U-Boot] [PATCH] treewide: compress lines for immediate return
Hi Masahiro, On 02/09/2016 12:36, Masahiro Yamada wrote: > -ret = expression; > -if (ret) > -return ret; > -return 0; > +return expression; > > Signed-off-by: Masahiro Yamada> --- > > arch/arm/cpu/armv7/bcm235xx/clk-bsc.c | 6 +- > arch/arm/cpu/armv7/bcm281xx/clk-bsc.c | 6 +- > arch/arm/mach-rockchip/rk3288/sdram_rk3288.c| 11 -- > arch/arm/mach-uniphier/dram/umc-ld20.c | 6 +- > arch/powerpc/lib/bootm.c| 6 +- > arch/x86/cpu/baytrail/valleyview.c | 8 +-- > arch/x86/cpu/broadwell/cpu.c| 6 +- > arch/x86/cpu/broadwell/sata.c | 7 +-- > arch/x86/cpu/ivybridge/bd82x6x.c| 6 +- > arch/x86/cpu/ivybridge/cpu.c| 6 +- > arch/x86/cpu/ivybridge/gma.c| 6 +- > arch/x86/cpu/ivybridge/ivybridge.c | 8 +-- > arch/x86/cpu/qemu/qemu.c| 8 +-- > arch/x86/cpu/queensbay/tnc.c| 14 ++--- > board/compulab/cm_fx6/cm_fx6.c | 6 +- > board/freescale/common/fsl_validate.c | 6 +- > board/freescale/mx6qsabreauto/mx6qsabreauto.c | 11 +++--- > board/freescale/mx6sxsabreauto/mx6sxsabreauto.c | 11 +++--- > board/keymile/km_arm/fpga_config.c | 14 ++--- > board/kosagi/novena/novena.c| 6 +- > board/samsung/goni/goni.c | 8 +-- > board/ti/common/board_detect.c | 6 +- > drivers/adc/adc-uclass.c| 12 ++- > drivers/core/root.c | 12 ++- > drivers/dfu/dfu_sf.c| 8 ++- > drivers/gpio/74x164_gpio.c | 7 +-- > drivers/gpio/axp_gpio.c | 6 +- > drivers/misc/cros_ec.c | 6 +- > drivers/misc/pca9551_led.c | 13 ++-- > drivers/misc/tegra186_bpmp.c| 6 +- > drivers/mmc/atmel_sdhci.c | 7 +-- > drivers/mmc/davinci_mmc.c | 6 +- > drivers/mmc/exynos_dw_mmc.c | 7 +-- > drivers/mmc/mmc-uclass.c| 7 +-- > drivers/mmc/mmc_boot.c | 28 > +++-- > drivers/mmc/mmc_legacy.c| 7 +-- > drivers/mmc/msm_sdhci.c | 7 +-- > drivers/mmc/mxcmmc.c| 6 +- > drivers/mmc/pxa_mmc_gen.c | 21 +++ > drivers/mmc/rockchip_dw_mmc.c | 7 +-- > drivers/mmc/rockchip_sdhci.c| 7 +-- > drivers/mmc/sandbox_mmc.c | 7 +-- > drivers/mmc/zynq_sdhci.c| 7 +-- > drivers/mtd/nand/fsl_elbc_nand.c| 6 +- > drivers/mtd/nand/fsl_ifc_nand.c | 5 + > drivers/mtd/nand/tegra_nand.c | 6 +- > drivers/mtd/nand/vf610_nfc.c| 6 +- > drivers/mtd/ubi/attach.c| 14 - > drivers/net/fm/eth.c| 6 +- > drivers/net/mvpp2.c | 16 -- > drivers/net/phy/mv88e6352.c | 6 +- > drivers/net/xilinx_emaclite.c | 7 +-- > drivers/pci/pcie_imx.c | 12 ++- > drivers/power/axp809.c | 8 +-- > drivers/rtc/i2c_rtc_emul.c | 13 ++-- > drivers/usb/host/ehci-atmel.c | 8 +-- > drivers/usb/host/ehci-fsl.c | 8 +-- > drivers/usb/host/ehci-marvell.c | 8 +-- > drivers/usb/host/ehci-mx6.c | 8 +-- > drivers/usb/host/ehci-pci.c | 8 +-- > drivers/usb/host/ehci-zynq.c| 8 +-- > drivers/usb/host/xhci-fsl.c | 7 +-- > drivers/usb/ulpi/ulpi.c | 6 +- > drivers/video/bridge/ptn3460.c | 8 +-- > drivers/video/rockchip/rk_edp.c | 6 +- > drivers/video/simple_panel.c| 7 +-- > drivers/video/tegra124/display.c| 8 +-- > drivers/video/vidconsole-uclass.c | 7 +-- > fs/ubifs/budget.c | 7 ++- > fs/ubifs/gc.c | 5 + > fs/ubifs/lpt_commit.c | 5 + > fs/ubifs/ubifs.c| 6 +- > lib/libfdt/fdt_rw.c | 6 +- > lib/rsa/rsa-checksum.c
Re: [U-Boot] [PATCH] treewide: compress lines for immediate return
Hello Masahiro, Am 02.09.2016 um 12:36 schrieb Masahiro Yamada: -ret = expression; -if (ret) -return ret; -return 0; +return expression; Signed-off-by: Masahiro Yamada--- [...] fs/ubifs/budget.c | 7 ++- fs/ubifs/gc.c | 5 + fs/ubifs/lpt_commit.c | 5 + fs/ubifs/ubifs.c| 6 +- Thanks! Acked-by: Heiko Schocher As the ubifs code comes from linux, I posted the parts for the ubifs on the linux-mtd mailinglist. bye, Heiko -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] treewide: compress lines for immediate return
On 09/04/2016 01:38 AM, Masahiro Yamada wrote: > 2016-09-02 23:12 GMT+09:00 Marek Vasut: >> On 09/02/2016 03:09 PM, Masahiro Yamada wrote: >>> 2016-09-02 20:58 GMT+09:00 Marek Vasut : On 09/02/2016 12:36 PM, Masahiro Yamada wrote: > -ret = expression; > -if (ret) > -return ret; > -return 0; > +return expression; > > Signed-off-by: Masahiro Yamada > --- The thing I miss in the commit message is -- why is this change beneficial/needed ? >>> >>> I thought the benefit was apparent. >>> >>> Wasn't it to you? >> >> Nope, please explain. >> > > This is a cleanup patch; it removes > unneeded variable assignments, if conditionals. This should be in the commit message. -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] treewide: compress lines for immediate return
2016-09-03 2:15 GMT+09:00 Stephen Warren: > On 09/02/2016 04:36 AM, Masahiro Yamada wrote: >> >> -ret = expression; >> -if (ret) >> -return ret; >> -return 0; >> +return expression; > > > I disagree with this change if applied blindly; I think both coding styles > have their merit depending on the semantic context. > > In the case of a simple wrapper function that just calls another with > different arguments, directly returning the result from the called function > make sense. For example: > >> int device_bind(struct udevice *parent, const struct driver *drv, >> const char *name, void *platdata, int of_offset, >> struct udevice **devp) >> { >> return device_bind_common(parent, drv, name, platdata, 0, >> of_offset, 0, >> devp); >> } > > > However, where the top-level function is more complex, especially where it > calls multiple functions in its body, I think it's best to use the exact > same style to call all functions in the top-level body, and just "return 0" > separately at the end. For example: > >> static int tegra186_bpmp_bind(struct udevice *dev) >> { >> int ret; >> struct udevice *child; >> >> debug("%s(dev=%p)\n", __func__, dev); >> >> ret = device_bind_driver_to_node(dev, "tegra186_clk", >> "tegra186_clk", >> dev->of_offset, ); >> if (ret) >> return ret; >> >> ret = device_bind_driver_to_node(dev, "tegra186_reset", >> "tegra186_reset", dev->of_offset, >> ); >> if (ret) >> return ret; >> >> ret = device_bind_driver_to_node(dev, "tegra186_power_domain", >> "tegra186_power_domain", >> dev->of_offset, ); >> if (ret) >> return ret; >> >> ret = dm_scan_fdt_dev(dev); >> if (ret) >> return ret; >> >> return 0; >> } > > > All child function calls are structured the same, so someone reading the > code will always see the same structure irrespective of where in a function > a child function is called. This gives uniformity. This also yields a few > maintenance advantages below, and helps keep all code uniform even if any of > the maintenance operations below have been applied to some functions and > aren't needed in others. Did you think I ran a semantic patch with Coccinelle and then sent it blindly? No, this patch passed my eyes' check, at least. Please notice this patch did not transform the following function in arch/arm/cpu/armv7/am33xx/clk_synthesizer.c int setup_clock_synthesizer(struct clk_synth *data) { int rc; uint8_t val; rc = i2c_probe(CLK_SYNTHESIZER_I2C_ADDR); if (rc) { printf("i2c probe failed at address 0x%x\n", CLK_SYNTHESIZER_I2C_ADDR); return rc; } rc = clk_synthesizer_reg_read(CLK_SYNTHESIZER_ID_REG, ); if (val != data->id) return rc; /* Crystal Load capacitor selection */ rc = clk_synthesizer_reg_write(CLK_SYNTHESIZER_XCSEL, data->capacitor); if (rc) return rc; rc = clk_synthesizer_reg_write(CLK_SYNTHESIZER_MUX_REG, data->mux); if (rc) return rc; rc = clk_synthesizer_reg_write(CLK_SYNTHESIZER_PDIV2_REG, data->pdiv2); if (rc) return rc; rc = clk_synthesizer_reg_write(CLK_SYNTHESIZER_PDIV3_REG, data->pdiv3); if (rc) return rc; return 0; } We can transform this function if we like but it would badly break the uniformity. So, I did not. So, I agree your statement to some extent. I guess the matter is, where we should draw a line. What is uniform and what is not uniform? If you want to stick to the safest side, a semantic patch like follows would do the job. @@ type t; expression e; identifier ret; @@ - t ret; - ret = e; - if (ret) - return ret; - return 0; + + return e; > 1) > > If tegra186_bpmp_bind() were modified to call an additional function in its > body, the diff for that addition would look identical no matter whether the > new call was added at the start/middle/end of the body. We wouldn't ever > have to do something like: > > - return dm_scan_fdt_dev(dev); > + ret = dm_scan_fdt_dev(dev); > + if (ret) > + return ret; > > ... which is an edit to a piece of code that's unrelated to the code being > added, and thus makes the patch more confusing. > > 2) > > There's always an obvious place to put any debug()/error() invocations, or > translate return values; inside the if (ret) body. There's only one way for > the code to look so it doesn't change based on
Re: [U-Boot] [PATCH] treewide: compress lines for immediate return
2016-09-02 23:12 GMT+09:00 Marek Vasut: > On 09/02/2016 03:09 PM, Masahiro Yamada wrote: >> 2016-09-02 20:58 GMT+09:00 Marek Vasut : >>> On 09/02/2016 12:36 PM, Masahiro Yamada wrote: -ret = expression; -if (ret) -return ret; -return 0; +return expression; Signed-off-by: Masahiro Yamada --- >>> >>> The thing I miss in the commit message is -- why is this change >>> beneficial/needed ? >>> >> >> I thought the benefit was apparent. >> >> Wasn't it to you? > > Nope, please explain. > This is a cleanup patch; it removes unneeded variable assignments, if conditionals. -- Best Regards Masahiro Yamada ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] treewide: compress lines for immediate return
On 09/02/2016 04:36 AM, Masahiro Yamada wrote: -ret = expression; -if (ret) -return ret; -return 0; +return expression; I disagree with this change if applied blindly; I think both coding styles have their merit depending on the semantic context. In the case of a simple wrapper function that just calls another with different arguments, directly returning the result from the called function make sense. For example: int device_bind(struct udevice *parent, const struct driver *drv, const char *name, void *platdata, int of_offset, struct udevice **devp) { return device_bind_common(parent, drv, name, platdata, 0, of_offset, 0, devp); } However, where the top-level function is more complex, especially where it calls multiple functions in its body, I think it's best to use the exact same style to call all functions in the top-level body, and just "return 0" separately at the end. For example: static int tegra186_bpmp_bind(struct udevice *dev) { int ret; struct udevice *child; debug("%s(dev=%p)\n", __func__, dev); ret = device_bind_driver_to_node(dev, "tegra186_clk", "tegra186_clk", dev->of_offset, ); if (ret) return ret; ret = device_bind_driver_to_node(dev, "tegra186_reset", "tegra186_reset", dev->of_offset, ); if (ret) return ret; ret = device_bind_driver_to_node(dev, "tegra186_power_domain", "tegra186_power_domain", dev->of_offset, ); if (ret) return ret; ret = dm_scan_fdt_dev(dev); if (ret) return ret; return 0; } All child function calls are structured the same, so someone reading the code will always see the same structure irrespective of where in a function a child function is called. This gives uniformity. This also yields a few maintenance advantages below, and helps keep all code uniform even if any of the maintenance operations below have been applied to some functions and aren't needed in others. 1) If tegra186_bpmp_bind() were modified to call an additional function in its body, the diff for that addition would look identical no matter whether the new call was added at the start/middle/end of the body. We wouldn't ever have to do something like: - return dm_scan_fdt_dev(dev); + ret = dm_scan_fdt_dev(dev); + if (ret) + return ret; ... which is an edit to a piece of code that's unrelated to the code being added, and thus makes the patch more confusing. 2) There's always an obvious place to put any debug()/error() invocations, or translate return values; inside the if (ret) body. There's only one way for the code to look so it doesn't change based on what exactly we do with the return value. 3) If we add the need for cleanup logic in the failure case, we can just blindly change "return ret" to "goto fail" without re-structuring code. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] treewide: compress lines for immediate return
2016-09-02 20:58 GMT+09:00 Marek Vasut: > On 09/02/2016 12:36 PM, Masahiro Yamada wrote: >> -ret = expression; >> -if (ret) >> -return ret; >> -return 0; >> +return expression; >> >> Signed-off-by: Masahiro Yamada >> --- > > The thing I miss in the commit message is -- why is this change > beneficial/needed ? > I thought the benefit was apparent. Wasn't it to you? -- Best Regards Masahiro Yamada ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] treewide: compress lines for immediate return
On 09/02/2016 03:09 PM, Masahiro Yamada wrote: > 2016-09-02 20:58 GMT+09:00 Marek Vasut: >> On 09/02/2016 12:36 PM, Masahiro Yamada wrote: >>> -ret = expression; >>> -if (ret) >>> -return ret; >>> -return 0; >>> +return expression; >>> >>> Signed-off-by: Masahiro Yamada >>> --- >> >> The thing I miss in the commit message is -- why is this change >> beneficial/needed ? >> > > I thought the benefit was apparent. > > Wasn't it to you? Nope, please explain. -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] treewide: compress lines for immediate return
On 09/02/2016 12:36 PM, Masahiro Yamada wrote: > -ret = expression; > -if (ret) > -return ret; > -return 0; > +return expression; > > Signed-off-by: Masahiro Yamada> --- The thing I miss in the commit message is -- why is this change beneficial/needed ? -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot