On 09/03/2016 05:38 PM, Masahiro Yamada wrote:
2016-09-03 2:15 GMT+09:00 Stephen Warren <[email protected]>:
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, &child);
if (ret)
return ret;
ret = device_bind_driver_to_node(dev, "tegra186_reset",
"tegra186_reset", dev->of_offset,
&child);
if (ret)
return ret;
ret = device_bind_driver_to_node(dev, "tegra186_power_domain",
"tegra186_power_domain",
dev->of_offset, &child);
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.
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
