[U-Boot] [PATCH] treewide: compress lines for immediate return

[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, &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.

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.