Re: [PATCH bluetooth-next 1/5] at86rf230: remove if branch

[Alexander Aring <alex.aring@gmail.com>]

On Mon, Dec 15, 2014 at 08:29:24AM +0000, Stefan Schmidt wrote:
> Hello.
> 
> On 15/12/14 00:20, Alexander Aring wrote:
> >This patch removes an unnecessary if branch inside the tx complete
> >handler.
> >
> >Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> >---
> >  drivers/net/ieee802154/at86rf230.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> >diff --git a/drivers/net/ieee802154/at86rf230.c
> >b/drivers/net/ieee802154/at86rf230.c
> >index 1c01356..4e983b3 100644
> >--- a/drivers/net/ieee802154/at86rf230.c
> >+++ b/drivers/net/ieee802154/at86rf230.c
> >@@ -715,10 +715,7 @@ at86rf230_tx_complete(void *context)
> >
> >  	enable_irq(lp->spi->irq);
> >
> >-	if (lp->max_frame_retries <= 0)
> >-		ieee802154_xmit_complete(lp->hw, skb, true);
> >-	else
> >-		ieee802154_xmit_complete(lp->hw, skb, false);
> >+	ieee802154_xmit_complete(lp->hw, skb, lp->max_frame_retries <= 0);
> >  }
> 
> It surely saves us some lines but personally I find it harder to read this
> way. Having the condition inside the function call breaks the reading flow
> for me. Is this the preferred coding style in the kernel?
> 

I agree this is more readable, maybe this is because I wrote this at
first time in this way.

But that reminds me to doing something like this:

if (bool)
	return true;
else
	return false;

instead:

return bool;

which is usually some beginners failure. I don't know if the kernel
conding style really discuss about that. The checkpatch script doesn't
report anything about this.


Maybe we can put something on the stack like:

bool ifs_handling = lp->max_frame_retries <= 0;

...

ieee802154_xmit_complete(lp->hw, skb, ifs_handling);

is that more readable?

- Alex