Re: [PATCH 8/8] mfd: lm3533: Simplify function return logic

[Johan Hovold]

On Thu, Oct 01, 2015 at 08:17:00AM +0100, Lee Jones wrote:
> On Wed, 30 Sep 2015, Johan Hovold wrote:
> 
> > On Tue, Sep 29, 2015 at 01:26:08PM +0200, Javier Martinez Canillas wrote:
> > > The invoked functions already return zero on success or a negative
> > > errno code so there is no need to open code the logic in the caller.
> > > 
> > > Signed-off-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
> > 
> > I do not consider this an improvement in any way and suggest this patch
> > is dropped.
> 
> Sorry Johan, but I disagree.

It's your call now, but I really think this is something that should be
up to the author of the code (which in this case happens to be me).

Neither style is incorrect, but there are reasons for preferring the
current style in this case.

> > > ---
> > >  drivers/mfd/lm3533-core.c | 12 ++----------
> > >  1 file changed, 2 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
> > > index 643f3750e830..193ecee1fa7e 100644
> > > --- a/drivers/mfd/lm3533-core.c
> > > +++ b/drivers/mfd/lm3533-core.c
> > > @@ -472,11 +472,7 @@ static int lm3533_device_setup(struct lm3533 *lm3533,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	ret = lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > > -	return 0;
> > > +	return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
> > 
> > You're saving a few lines of code but instead introduce asymmetries and
> > obscure the fact that the function returns zero on success.
> 
> There is no obfuscation here.  Functions normally return zero on
> success and !zero on failure, it's what's expected.

We have functions returning boolean true or a positive integer on
success.

In summary (rehashing what I wrote in a different thread) I do think
that:

{
        int ret;

        ret = init_a(...);
        if (ret)
                return ret;

        ret = init_b(...);
        if (ret)
                return ret;

        return 0;
}

is (at least to me) preferred over:

{
        int ret;

        ret = init_a(...);
        if (ret)
                return ret;

        return init_b(...);
}

for symmetry and readability reasons (e.g. I don't have to look at
init_b to figure out what the functions returns). And with a long
parameter list to init_b with line breaks, this would look even worse.

But either way, it should be up to the author of the code to decide what
style to use.

> I'm going to apply the patch.

Fair enough. I've asked for the warnings to be removed from coccinelle
so hopefully we can get on to fix real issues. ;)

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/