Re: [Outreachy} [PATCH] Staging: vt6655: Refactor switch case statement.

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Mar 24, 2020 at 09:55:00PM -0700, Briana Oursler wrote:
> Replaces nested if-else within switch case with ternary logic and variable
> setting to limit returns. Exposes primary logic to aid
> readability.

Wrap your changelog at the same size for each line, don't make it up :)

> 
> Signed-off-by: Briana Oursler <briana.oursler@gmail.com>
> ---
>  drivers/staging/vt6655/rxtx.c | 101 +++++++++++++---------------------
>  1 file changed, 39 insertions(+), 62 deletions(-)

Where did the "}" character come from on the subject line?


> 
> diff --git a/drivers/staging/vt6655/rxtx.c b/drivers/staging/vt6655/rxtx.c
> index 37fcc42ed000..eb7b20286ae2 100644
> --- a/drivers/staging/vt6655/rxtx.c
> +++ b/drivers/staging/vt6655/rxtx.c
> @@ -240,6 +240,7 @@ s_uGetDataDuration(
>  {
>  	bool bLastFrag = false;
>  	unsigned int uAckTime = 0, uNextPktTime = 0;
> +	unsigned int len;
>  
>  	if (uFragIdx == (uMACfragNum - 1))
>  		bLastFrag = true;
> @@ -254,17 +255,18 @@ s_uGetDataDuration(
>  				return 0;
>  			}
>  		} else {/* First Frag or Mid Frag */
> -			if (uFragIdx == (uMACfragNum - 2))
> -				uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wRate, bNeedAck);
> -			else
> -				uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wRate, bNeedAck);
> +			len = (uMacfragNum - 2 == uFragIdx) ?
> +				cbLastFragmentSize : cbFrameLength;

Ugh, I hate ? : logic.

Spell it out, be specific, this is just much harder to read now than
before.

> +			uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, len, wRate, bNeedAck);
>  
>  			if (bNeedAck) {
> -				uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopCCKBasicRate);
> -				return pDevice->uSIFS + uAckTime + uNextPktTime;
> +				uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
> +							   byPktType, 14, pDevice->byTopCCKBasicRate);

You didn't wrap this properly?


>  			} else {
> -				return pDevice->uSIFS + uNextPktTime;
> +				uAckTime = 0;
>  			}
> +
> +			return pDevice->uSIFS + uAckTime + uNextPktTime;
>  		}
>  		break;
>  
> @@ -277,17 +279,18 @@ s_uGetDataDuration(
>  				return 0;
>  			}
>  		} else {/* First Frag or Mid Frag */
> -			if (uFragIdx == (uMACfragNum - 2))
> -				uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wRate, bNeedAck);
> -			else
> -				uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wRate, bNeedAck);
> +			len = (uMacfragNum - 2 == uFragIdx) ?
> +				cbLastFragmentSize : cbFrameLength;

Same here, this makes it harder to understand.

thanks,

greg k-h