* Re: [Outreachy] [PATCH] Staging: vt6655: Refactor switch case statement.
2020-03-25 12:35 ` Greg KH
@ 2020-03-25 17:46 ` Briana Oursler
2020-03-25 17:48 ` Briana Oursler
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Briana Oursler @ 2020-03-25 17:46 UTC (permalink / raw)
To: Greg KH; +Cc: forest, outreachy-kernel
On Wed, 2020-03-25 at 13:35 +0100, Greg KH wrote:
> 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
> :)
Thanks, I will avoid wrapping them manually going forward.
>
> > 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.
I'll remove the ternary logic. Would it be better to use a bool and
if/else to determine the uNextPktTime? I think changing it to a bool
instead would have the advantage that it would match a strategy used
elsewhere on that same switch case with bNeedAck. Should I perhaps
resubmit using that strategy? Or would it be better for me to either
leave this one as is or go back to the drawing board on it?
>
> > + 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?
>
Sorry, I'll fix that.
>
> > } 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
Thanks for the input, I appreciate it.
--Briana
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Outreachy] [PATCH] Staging: vt6655: Refactor switch case statement.
2020-03-25 12:35 ` Greg KH
2020-03-25 17:46 ` [Outreachy] " Briana Oursler
@ 2020-03-25 17:48 ` Briana Oursler
2020-03-25 18:01 ` Briana Oursler
2020-03-27 0:33 ` [PATCH v2] " Briana Oursler
3 siblings, 0 replies; 15+ messages in thread
From: Briana Oursler @ 2020-03-25 17:48 UTC (permalink / raw)
To: Greg KH; +Cc: forest, outreachy-kernel
On Wed, 2020-03-25 at 13:35 +0100, Greg KH wrote:
> 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
> :)
Thanks, I will avoid wrapping them manually going forward.
>
> > 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.
I'll remove the ternary logic. Would it be better to use a bool and
if/else to determine the uNextPktTime? I think changing it to a bool
instead would have the advantage that it would match a strategy used
elsewhere on that same switch case with bNeedAck. Should I perhaps
resubmit using that strategy? Or would it be better for me to either
leave this one as is or go back to the drawing board on it?
>
> > + 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?
>
Sorry, I'll fix that.
>
> > } 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
Thanks for the input, I appreciate it.
--Briana
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Outreachy] [PATCH] Staging: vt6655: Refactor switch case statement.
2020-03-25 12:35 ` Greg KH
2020-03-25 17:46 ` [Outreachy] " Briana Oursler
2020-03-25 17:48 ` Briana Oursler
@ 2020-03-25 18:01 ` Briana Oursler
2020-03-25 19:34 ` Briana Oursler
2020-03-27 0:33 ` [PATCH v2] " Briana Oursler
3 siblings, 1 reply; 15+ messages in thread
From: Briana Oursler @ 2020-03-25 18:01 UTC (permalink / raw)
To: Greg KH; +Cc: forest, outreachy-kernel
On Wed, 2020-03-25 at 13:35 +0100, Greg KH wrote:
> 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
> :)
Thanks, I will avoid wrapping them manually going forward.
>
> > 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.
I'll remove the ternary logic. Would it be better to use a bool and
if/else to determine the uNextPktTime? I think changing it to a bool
instead would have the advantage that it would match a strategy used
elsewhere on that same switch case with bNeedAck. Should I perhaps
resubmit using that strategy? Or would it be better for me to either
leave this one as is or go back to the drawing board on it?
>
> > + 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?
>
Sorry, I'll fix that.
>
> > } 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
Thanks for the input, I appreciate it.
--Briana
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Outreachy] [PATCH] Staging: vt6655: Refactor switch case statement.
2020-03-25 18:01 ` Briana Oursler
@ 2020-03-25 19:34 ` Briana Oursler
0 siblings, 0 replies; 15+ messages in thread
From: Briana Oursler @ 2020-03-25 19:34 UTC (permalink / raw)
To: Greg KH; +Cc: forest, outreachy-kernel
On Wed, 2020-03-25 at 11:01 -0700, Briana Oursler wrote:
> On Wed, 2020-03-25 at 13:35 +0100, Greg KH wrote:
> > 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
> > :)
>
> Thanks, I will avoid wrapping them manually going forward.
>
> > > 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.
>
> I'll remove the ternary logic. Would it be better to use a bool and
> if/else to determine the uNextPktTime? I think changing it to a bool
> instead would have the advantage that it would match a strategy used
> elsewhere on that same switch case with bNeedAck. Should I perhaps
> resubmit using that strategy? Or would it be better for me to either
> leave this one as is or go back to the drawing board on it?
>
> > > + 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?
> >
>
> Sorry, I'll fix that.
>
> > > } 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
>
> Thanks for the input, I appreciate it.
>
> --Briana
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] Staging: vt6655: Refactor switch case statement.
2020-03-25 12:35 ` Greg KH
` (2 preceding siblings ...)
2020-03-25 18:01 ` Briana Oursler
@ 2020-03-27 0:33 ` Briana Oursler
2020-03-27 1:14 ` [Outreachy kernel] " Lakshmi Ramasubramanian
3 siblings, 1 reply; 15+ messages in thread
From: Briana Oursler @ 2020-03-27 0:33 UTC (permalink / raw)
To: gregkh, forest, outreachy-kernel; +Cc: Briana Oursler
Pulls some logic branches above switch case and holds outcomes in
variables to improve readability and reduce code duplication within
switch case.
Signed-off-by: Briana Oursler <briana.oursler@gmail.com>
---
Changes in v2:
- Removes ternary logic.
switch case.
- Introduces 'flag' bool.
- Moves len definition above switch case.
drivers/staging/vt6655/rxtx.c | 128 ++++++++++++++++++----------------
1 file changed, 66 insertions(+), 62 deletions(-)
diff --git a/drivers/staging/vt6655/rxtx.c b/drivers/staging/vt6655/rxtx.c
index 37fcc42ed000..5b45d66b7f39 100644
--- a/drivers/staging/vt6655/rxtx.c
+++ b/drivers/staging/vt6655/rxtx.c
@@ -239,11 +239,21 @@ s_uGetDataDuration(
)
{
bool bLastFrag = false;
+ bool flag = false;
unsigned int uAckTime = 0, uNextPktTime = 0;
+ unsigned int len;
if (uFragIdx == (uMACfragNum - 1))
bLastFrag = true;
+ if (uFragIdx == (uMACfragNum - 2))
+ len = cbLastFragmentSize;
+ else
+ len = cbFrameLength;
+
+ if (byFBOption == AUTO_FB_0)
+ flag = true;
+
switch (byDurType) {
case DATADUR_B: /* DATADUR_B */
if (((uMACfragNum == 1)) || bLastFrag) {/* Non Frag or Last Frag */
@@ -254,17 +264,17 @@ 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);
+ 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);
} else {
- return pDevice->uSIFS + uNextPktTime;
+ uAckTime = 0;
}
+
+ return pDevice->uSIFS + uAckTime + uNextPktTime;
}
break;
@@ -277,17 +287,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);
+ uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, len,
+ wRate, bNeedAck);
if (bNeedAck) {
- uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
- return pDevice->uSIFS + uAckTime + uNextPktTime;
+ uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
+ byPktType, 14,
+ pDevice->byTopOFDMBasicRate);
} else {
- return pDevice->uSIFS + uNextPktTime;
+ uAckTime = 0;
}
+
+ return pDevice->uSIFS + uAckTime + uNextPktTime;
}
break;
@@ -300,35 +311,33 @@ s_uGetDataDuration(
return 0;
}
} else { /* First Frag or Mid Frag */
- if (byFBOption == AUTO_FB_0) {
- if (wRate < RATE_18M)
- wRate = RATE_18M;
- else if (wRate > RATE_54M)
- wRate = RATE_54M;
-
- if (uFragIdx == (uMACfragNum - 2))
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt0[FB_RATE0][wRate-RATE_18M], bNeedAck);
- else
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE0][wRate-RATE_18M], bNeedAck);
-
- } else { /* (byFBOption == AUTO_FB_1) */
- if (wRate < RATE_18M)
- wRate = RATE_18M;
- else if (wRate > RATE_54M)
- wRate = RATE_54M;
-
- if (uFragIdx == (uMACfragNum - 2))
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt1[FB_RATE0][wRate-RATE_18M], bNeedAck);
- else
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt1[FB_RATE0][wRate-RATE_18M], bNeedAck);
+ if (wRate < RATE_18M)
+ wRate = RATE_18M;
+ else if (wRate > RATE_54M)
+ wRate = RATE_54M;
+
+ /* flag true if (byFBOption == AUTO_FB_0) */
+ if (flag) {
+ uNextPktTime = s_uGetTxRsvTime(pDevice,
+ byPktType, len,
+ wFB_Opt0[FB_RATE0][wRate - RATE_18M],
+ bNeedAck);
+ } else {
+ uNextPktTime = s_uGetTxRsvTime(pDevice,
+ byPktType, len,
+ wFB_Opt1[FB_RATE0][wRate - RATE_18M],
+ bNeedAck);
}
if (bNeedAck) {
- uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
- return pDevice->uSIFS + uAckTime + uNextPktTime;
+ uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
+ byPktType, 14,
+ pDevice->byTopOFDMBasicRate);
} else {
- return pDevice->uSIFS + uNextPktTime;
+ uAckTime = 0;
}
+
+ return pDevice->uSIFS + uAckTime + uNextPktTime;
}
break;
@@ -341,34 +350,29 @@ s_uGetDataDuration(
return 0;
}
} else { /* First Frag or Mid Frag */
- if (byFBOption == AUTO_FB_0) {
- if (wRate < RATE_18M)
- wRate = RATE_18M;
- else if (wRate > RATE_54M)
- wRate = RATE_54M;
-
- if (uFragIdx == (uMACfragNum - 2))
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt0[FB_RATE1][wRate-RATE_18M], bNeedAck);
- else
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE1][wRate-RATE_18M], bNeedAck);
-
- } else { /* (byFBOption == AUTO_FB_1) */
- if (wRate < RATE_18M)
- wRate = RATE_18M;
- else if (wRate > RATE_54M)
- wRate = RATE_54M;
-
- if (uFragIdx == (uMACfragNum - 2))
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt1[FB_RATE1][wRate-RATE_18M], bNeedAck);
- else
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt1[FB_RATE1][wRate-RATE_18M], bNeedAck);
+
+ /* flag true if (byFBOption == AUTO_FB_0) */
+ if (flag) {
+ uNextPktTime = s_uGetTxRsvTime(pDevice,
+ byPktType, len,
+ wFB_Opt0[FB_RATE0][wRate - RATE_18M],
+ bNeedAck);
+ } else {
+ uNextPktTime = s_uGetTxRsvTime(pDevice,
+ byPktType, len,
+ wFB_Opt1[FB_RATE0][wRate - RATE_18M],
+ bNeedAck);
}
+
if (bNeedAck) {
- uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
- return pDevice->uSIFS + uAckTime + uNextPktTime;
+ uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
+ byPktType, 14,
+ pDevice->byTopOFDMBasicRate);
} else {
- return pDevice->uSIFS + uNextPktTime;
+ uAckTime = 0;
}
+
+ return pDevice->uSIFS + uAckTime + uNextPktTime;
}
break;
--
2.24.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Outreachy kernel] [PATCH v2] Staging: vt6655: Refactor switch case statement.
2020-03-27 0:33 ` [PATCH v2] " Briana Oursler
@ 2020-03-27 1:14 ` Lakshmi Ramasubramanian
2020-03-27 2:53 ` [Outreachy][PATCH v3] " Briana Oursler
0 siblings, 1 reply; 15+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-03-27 1:14 UTC (permalink / raw)
To: Briana Oursler, gregkh, forest, outreachy-kernel
On 3/26/20 5:33 PM, Briana Oursler wrote:
Hi Briana,
> Pulls some logic branches above switch case and holds outcomes in
> variables to improve readability and reduce code duplication within
> switch case.
I have copy & pasted the guidance Julia Lawall had pointed to on how
"subject" and "patch description" should be written. Please see below:
https://kernelnewbies.org/PatchPhilosophy suggests:
In patch descriptions and in the subject, it is common and preferable to
use present-tense, imperative language. Write as if you are telling git
what to do with your patch.
It provides the following as an example of a good description:
somedriver: fix sleep while atomic in send_a_packet()
The send_a_packet() function is called in atomic context but takes a
mutex, causing a sleeping while atomic warning. Change the skb_lock
to be a spinlock instead of a mutex to fix.
thanks,
-lakshmi
^ permalink raw reply [flat|nested] 15+ messages in thread* [Outreachy][PATCH v3] Staging: vt6655: Refactor switch case statement.
2020-03-27 1:14 ` [Outreachy kernel] " Lakshmi Ramasubramanian
@ 2020-03-27 2:53 ` Briana Oursler
2020-03-27 9:00 ` Greg KH
0 siblings, 1 reply; 15+ messages in thread
From: Briana Oursler @ 2020-03-27 2:53 UTC (permalink / raw)
To: nramas, gregkh, forest, outreachy-kernel; +Cc: Briana Oursler
Pull some logic branches above switch case. Create variables to reduce
code duplication and improve readability.
Signed-off-by: Briana Oursler <briana.oursler@gmail.com>
---
Changes in v3:
- Update grammar and meaning in patch description.
Changes in v2:
- Remove ternary logic in switch case.
- Introduce 'flag' bool.
- Move len definition above switch case.
drivers/staging/vt6655/rxtx.c | 128 ++++++++++++++++++----------------
1 file changed, 66 insertions(+), 62 deletions(-)
diff --git a/drivers/staging/vt6655/rxtx.c b/drivers/staging/vt6655/rxtx.c
index 37fcc42ed000..5b45d66b7f39 100644
--- a/drivers/staging/vt6655/rxtx.c
+++ b/drivers/staging/vt6655/rxtx.c
@@ -239,11 +239,21 @@ s_uGetDataDuration(
)
{
bool bLastFrag = false;
+ bool flag = false;
unsigned int uAckTime = 0, uNextPktTime = 0;
+ unsigned int len;
if (uFragIdx == (uMACfragNum - 1))
bLastFrag = true;
+ if (uFragIdx == (uMACfragNum - 2))
+ len = cbLastFragmentSize;
+ else
+ len = cbFrameLength;
+
+ if (byFBOption == AUTO_FB_0)
+ flag = true;
+
switch (byDurType) {
case DATADUR_B: /* DATADUR_B */
if (((uMACfragNum == 1)) || bLastFrag) {/* Non Frag or Last Frag */
@@ -254,17 +264,17 @@ 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);
+ 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);
} else {
- return pDevice->uSIFS + uNextPktTime;
+ uAckTime = 0;
}
+
+ return pDevice->uSIFS + uAckTime + uNextPktTime;
}
break;
@@ -277,17 +287,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);
+ uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, len,
+ wRate, bNeedAck);
if (bNeedAck) {
- uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
- return pDevice->uSIFS + uAckTime + uNextPktTime;
+ uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
+ byPktType, 14,
+ pDevice->byTopOFDMBasicRate);
} else {
- return pDevice->uSIFS + uNextPktTime;
+ uAckTime = 0;
}
+
+ return pDevice->uSIFS + uAckTime + uNextPktTime;
}
break;
@@ -300,35 +311,33 @@ s_uGetDataDuration(
return 0;
}
} else { /* First Frag or Mid Frag */
- if (byFBOption == AUTO_FB_0) {
- if (wRate < RATE_18M)
- wRate = RATE_18M;
- else if (wRate > RATE_54M)
- wRate = RATE_54M;
-
- if (uFragIdx == (uMACfragNum - 2))
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt0[FB_RATE0][wRate-RATE_18M], bNeedAck);
- else
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE0][wRate-RATE_18M], bNeedAck);
-
- } else { /* (byFBOption == AUTO_FB_1) */
- if (wRate < RATE_18M)
- wRate = RATE_18M;
- else if (wRate > RATE_54M)
- wRate = RATE_54M;
-
- if (uFragIdx == (uMACfragNum - 2))
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt1[FB_RATE0][wRate-RATE_18M], bNeedAck);
- else
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt1[FB_RATE0][wRate-RATE_18M], bNeedAck);
+ if (wRate < RATE_18M)
+ wRate = RATE_18M;
+ else if (wRate > RATE_54M)
+ wRate = RATE_54M;
+
+ /* flag true if (byFBOption == AUTO_FB_0) */
+ if (flag) {
+ uNextPktTime = s_uGetTxRsvTime(pDevice,
+ byPktType, len,
+ wFB_Opt0[FB_RATE0][wRate - RATE_18M],
+ bNeedAck);
+ } else {
+ uNextPktTime = s_uGetTxRsvTime(pDevice,
+ byPktType, len,
+ wFB_Opt1[FB_RATE0][wRate - RATE_18M],
+ bNeedAck);
}
if (bNeedAck) {
- uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
- return pDevice->uSIFS + uAckTime + uNextPktTime;
+ uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
+ byPktType, 14,
+ pDevice->byTopOFDMBasicRate);
} else {
- return pDevice->uSIFS + uNextPktTime;
+ uAckTime = 0;
}
+
+ return pDevice->uSIFS + uAckTime + uNextPktTime;
}
break;
@@ -341,34 +350,29 @@ s_uGetDataDuration(
return 0;
}
} else { /* First Frag or Mid Frag */
- if (byFBOption == AUTO_FB_0) {
- if (wRate < RATE_18M)
- wRate = RATE_18M;
- else if (wRate > RATE_54M)
- wRate = RATE_54M;
-
- if (uFragIdx == (uMACfragNum - 2))
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt0[FB_RATE1][wRate-RATE_18M], bNeedAck);
- else
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE1][wRate-RATE_18M], bNeedAck);
-
- } else { /* (byFBOption == AUTO_FB_1) */
- if (wRate < RATE_18M)
- wRate = RATE_18M;
- else if (wRate > RATE_54M)
- wRate = RATE_54M;
-
- if (uFragIdx == (uMACfragNum - 2))
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt1[FB_RATE1][wRate-RATE_18M], bNeedAck);
- else
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt1[FB_RATE1][wRate-RATE_18M], bNeedAck);
+
+ /* flag true if (byFBOption == AUTO_FB_0) */
+ if (flag) {
+ uNextPktTime = s_uGetTxRsvTime(pDevice,
+ byPktType, len,
+ wFB_Opt0[FB_RATE0][wRate - RATE_18M],
+ bNeedAck);
+ } else {
+ uNextPktTime = s_uGetTxRsvTime(pDevice,
+ byPktType, len,
+ wFB_Opt1[FB_RATE0][wRate - RATE_18M],
+ bNeedAck);
}
+
if (bNeedAck) {
- uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
- return pDevice->uSIFS + uAckTime + uNextPktTime;
+ uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
+ byPktType, 14,
+ pDevice->byTopOFDMBasicRate);
} else {
- return pDevice->uSIFS + uNextPktTime;
+ uAckTime = 0;
}
+
+ return pDevice->uSIFS + uAckTime + uNextPktTime;
}
break;
--
2.24.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Outreachy][PATCH v3] Staging: vt6655: Refactor switch case statement.
2020-03-27 2:53 ` [Outreachy][PATCH v3] " Briana Oursler
@ 2020-03-27 9:00 ` Greg KH
2020-03-27 21:32 ` [Outreachy][Patch v4] " Briana Oursler
0 siblings, 1 reply; 15+ messages in thread
From: Greg KH @ 2020-03-27 9:00 UTC (permalink / raw)
To: Briana Oursler; +Cc: nramas, forest, outreachy-kernel
On Thu, Mar 26, 2020 at 07:53:01PM -0700, Briana Oursler wrote:
> Pull some logic branches above switch case. Create variables to reduce
> code duplication and improve readability.
>
> Signed-off-by: Briana Oursler <briana.oursler@gmail.com>
> ---
>
> Changes in v3:
> - Update grammar and meaning in patch description.
>
> Changes in v2:
> - Remove ternary logic in switch case.
> - Introduce 'flag' bool.
> - Move len definition above switch case.
>
> drivers/staging/vt6655/rxtx.c | 128 ++++++++++++++++++----------------
> 1 file changed, 66 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/staging/vt6655/rxtx.c b/drivers/staging/vt6655/rxtx.c
> index 37fcc42ed000..5b45d66b7f39 100644
> --- a/drivers/staging/vt6655/rxtx.c
> +++ b/drivers/staging/vt6655/rxtx.c
> @@ -239,11 +239,21 @@ s_uGetDataDuration(
> )
> {
> bool bLastFrag = false;
> + bool flag = false;
> unsigned int uAckTime = 0, uNextPktTime = 0;
> + unsigned int len;
>
> if (uFragIdx == (uMACfragNum - 1))
> bLastFrag = true;
>
> + if (uFragIdx == (uMACfragNum - 2))
> + len = cbLastFragmentSize;
> + else
> + len = cbFrameLength;
> +
> + if (byFBOption == AUTO_FB_0)
> + flag = true;
"flag" is horrible to try to remember what it really is for.
You already know this, because you say so in your code change:
> + /* flag true if (byFBOption == AUTO_FB_0) */
> + if (flag) {
Why not just test for the == statement here?
Also, your comment is indented incorrectly, didn't checkpatch catch that
when you ran it against your patch?
> + /* flag true if (byFBOption == AUTO_FB_0) */
> + if (flag) {
Same goes here as well.
I don't see why adding that new variable saved any space or logic, it
only made the code harder to follow and forced you to write the logic
out anyway, in the comment :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 15+ messages in thread* [Outreachy][Patch v4] Staging: vt6655: Refactor switch case statement.
2020-03-27 9:00 ` Greg KH
@ 2020-03-27 21:32 ` Briana Oursler
2020-03-27 21:40 ` [Outreachy kernel] " Julia Lawall
0 siblings, 1 reply; 15+ messages in thread
From: Briana Oursler @ 2020-03-27 21:32 UTC (permalink / raw)
To: gregkh, nramas, forest, outreachy-kernel; +Cc: Briana Oursler
Simplify switch case statement to improve readability and limit code
duplication. Define len variable for next packet time function input.
Lift logic out of nested if else. Format long lines.
Signed-off-by: Briana Oursler <briana.oursler@gmail.com>
---
Changes in v4:
- Remove 'flag' bool and return comparison to switch case.
- Remove comment.
- Update patch description to better describe changes.
Changes in v3:
- Update grammar and meaning in patch description.
Changes in v2:
- Remove ternary logic in switch case.
- Introduce 'flag' bool.
- Move len definition above switch case.
drivers/staging/vt6655/rxtx.c | 118 +++++++++++++++++-----------------
1 file changed, 58 insertions(+), 60 deletions(-)
diff --git a/drivers/staging/vt6655/rxtx.c b/drivers/staging/vt6655/rxtx.c
index 37fcc42ed000..9a6abe8613e6 100644
--- a/drivers/staging/vt6655/rxtx.c
+++ b/drivers/staging/vt6655/rxtx.c
@@ -240,10 +240,16 @@ s_uGetDataDuration(
{
bool bLastFrag = false;
unsigned int uAckTime = 0, uNextPktTime = 0;
+ unsigned int len;
if (uFragIdx == (uMACfragNum - 1))
bLastFrag = true;
+ if (uFragIdx == (uMACfragNum - 2))
+ len = cbLastFragmentSize;
+ else
+ len = cbFrameLength;
+
switch (byDurType) {
case DATADUR_B: /* DATADUR_B */
if (((uMACfragNum == 1)) || bLastFrag) {/* Non Frag or Last Frag */
@@ -254,17 +260,17 @@ 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);
+ 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);
} else {
- return pDevice->uSIFS + uNextPktTime;
+ uAckTime = 0;
}
+
+ return pDevice->uSIFS + uAckTime + uNextPktTime;
}
break;
@@ -277,17 +283,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);
+ uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, len,
+ wRate, bNeedAck);
if (bNeedAck) {
- uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
- return pDevice->uSIFS + uAckTime + uNextPktTime;
+ uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
+ byPktType, 14,
+ pDevice->byTopOFDMBasicRate);
} else {
- return pDevice->uSIFS + uNextPktTime;
+ uAckTime = 0;
}
+
+ return pDevice->uSIFS + uAckTime + uNextPktTime;
}
break;
@@ -300,35 +307,32 @@ s_uGetDataDuration(
return 0;
}
} else { /* First Frag or Mid Frag */
+ if (wRate < RATE_18M)
+ wRate = RATE_18M;
+ else if (wRate > RATE_54M)
+ wRate = RATE_54M;
+
if (byFBOption == AUTO_FB_0) {
- if (wRate < RATE_18M)
- wRate = RATE_18M;
- else if (wRate > RATE_54M)
- wRate = RATE_54M;
-
- if (uFragIdx == (uMACfragNum - 2))
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt0[FB_RATE0][wRate-RATE_18M], bNeedAck);
- else
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE0][wRate-RATE_18M], bNeedAck);
-
- } else { /* (byFBOption == AUTO_FB_1) */
- if (wRate < RATE_18M)
- wRate = RATE_18M;
- else if (wRate > RATE_54M)
- wRate = RATE_54M;
-
- if (uFragIdx == (uMACfragNum - 2))
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt1[FB_RATE0][wRate-RATE_18M], bNeedAck);
- else
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt1[FB_RATE0][wRate-RATE_18M], bNeedAck);
+ uNextPktTime = s_uGetTxRsvTime(pDevice,
+ byPktType, len,
+ wFB_Opt0[FB_RATE0][wRate - RATE_18M],
+ bNeedAck);
+ } else {
+ uNextPktTime = s_uGetTxRsvTime(pDevice,
+ byPktType, len,
+ wFB_Opt1[FB_RATE0][wRate - RATE_18M],
+ bNeedAck);
}
if (bNeedAck) {
- uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
- return pDevice->uSIFS + uAckTime + uNextPktTime;
+ uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
+ byPktType, 14,
+ pDevice->byTopOFDMBasicRate);
} else {
- return pDevice->uSIFS + uNextPktTime;
+ uAckTime = 0;
}
+
+ return pDevice->uSIFS + uAckTime + uNextPktTime;
}
break;
@@ -341,34 +345,28 @@ s_uGetDataDuration(
return 0;
}
} else { /* First Frag or Mid Frag */
+
if (byFBOption == AUTO_FB_0) {
- if (wRate < RATE_18M)
- wRate = RATE_18M;
- else if (wRate > RATE_54M)
- wRate = RATE_54M;
-
- if (uFragIdx == (uMACfragNum - 2))
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt0[FB_RATE1][wRate-RATE_18M], bNeedAck);
- else
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE1][wRate-RATE_18M], bNeedAck);
-
- } else { /* (byFBOption == AUTO_FB_1) */
- if (wRate < RATE_18M)
- wRate = RATE_18M;
- else if (wRate > RATE_54M)
- wRate = RATE_54M;
-
- if (uFragIdx == (uMACfragNum - 2))
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt1[FB_RATE1][wRate-RATE_18M], bNeedAck);
- else
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt1[FB_RATE1][wRate-RATE_18M], bNeedAck);
+ uNextPktTime = s_uGetTxRsvTime(pDevice,
+ byPktType, len,
+ wFB_Opt0[FB_RATE0][wRate - RATE_18M],
+ bNeedAck);
+ } else {
+ uNextPktTime = s_uGetTxRsvTime(pDevice,
+ byPktType, len,
+ wFB_Opt1[FB_RATE0][wRate - RATE_18M],
+ bNeedAck);
}
+
if (bNeedAck) {
- uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
- return pDevice->uSIFS + uAckTime + uNextPktTime;
+ uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
+ byPktType, 14,
+ pDevice->byTopOFDMBasicRate);
} else {
- return pDevice->uSIFS + uNextPktTime;
+ uAckTime = 0;
}
+
+ return pDevice->uSIFS + uAckTime + uNextPktTime;
}
break;
--
2.24.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Outreachy kernel] [Outreachy][Patch v4] Staging: vt6655: Refactor switch case statement.
2020-03-27 21:32 ` [Outreachy][Patch v4] " Briana Oursler
@ 2020-03-27 21:40 ` Julia Lawall
2020-03-28 5:10 ` [patch v5] Staging: vt6655: Reduce statement complexity Briana Oursler
0 siblings, 1 reply; 15+ messages in thread
From: Julia Lawall @ 2020-03-27 21:40 UTC (permalink / raw)
To: Briana Oursler; +Cc: gregkh, nramas, forest, outreachy-kernel
On Fri, 27 Mar 2020, Briana Oursler wrote:
> Simplify switch case statement to improve readability and limit code
> duplication. Define len variable for next packet time function input.
> Lift logic out of nested if else. Format long lines.
The subject and log message seem very misleading. You don't seem to
modify either a switch or a case. I guess you mean that you are focusing
on the branches of a single switch statement. But the switch doesn't seem
like the important part. The log message lists a variety of things that
are done. What is the main theme of the patch?
julia
>
> Signed-off-by: Briana Oursler <briana.oursler@gmail.com>
> ---
>
> Changes in v4:
> - Remove 'flag' bool and return comparison to switch case.
> - Remove comment.
> - Update patch description to better describe changes.
>
> Changes in v3:
> - Update grammar and meaning in patch description.
>
> Changes in v2:
> - Remove ternary logic in switch case.
> - Introduce 'flag' bool.
> - Move len definition above switch case.
>
> drivers/staging/vt6655/rxtx.c | 118 +++++++++++++++++-----------------
> 1 file changed, 58 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/staging/vt6655/rxtx.c b/drivers/staging/vt6655/rxtx.c
> index 37fcc42ed000..9a6abe8613e6 100644
> --- a/drivers/staging/vt6655/rxtx.c
> +++ b/drivers/staging/vt6655/rxtx.c
> @@ -240,10 +240,16 @@ s_uGetDataDuration(
> {
> bool bLastFrag = false;
> unsigned int uAckTime = 0, uNextPktTime = 0;
> + unsigned int len;
>
> if (uFragIdx == (uMACfragNum - 1))
> bLastFrag = true;
>
> + if (uFragIdx == (uMACfragNum - 2))
> + len = cbLastFragmentSize;
> + else
> + len = cbFrameLength;
> +
> switch (byDurType) {
> case DATADUR_B: /* DATADUR_B */
> if (((uMACfragNum == 1)) || bLastFrag) {/* Non Frag or Last Frag */
> @@ -254,17 +260,17 @@ 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);
> + 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);
> } else {
> - return pDevice->uSIFS + uNextPktTime;
> + uAckTime = 0;
> }
> +
> + return pDevice->uSIFS + uAckTime + uNextPktTime;
> }
> break;
>
> @@ -277,17 +283,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);
> + uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, len,
> + wRate, bNeedAck);
>
> if (bNeedAck) {
> - uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
> - return pDevice->uSIFS + uAckTime + uNextPktTime;
> + uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
> + byPktType, 14,
> + pDevice->byTopOFDMBasicRate);
> } else {
> - return pDevice->uSIFS + uNextPktTime;
> + uAckTime = 0;
> }
> +
> + return pDevice->uSIFS + uAckTime + uNextPktTime;
> }
> break;
>
> @@ -300,35 +307,32 @@ s_uGetDataDuration(
> return 0;
> }
> } else { /* First Frag or Mid Frag */
> + if (wRate < RATE_18M)
> + wRate = RATE_18M;
> + else if (wRate > RATE_54M)
> + wRate = RATE_54M;
> +
> if (byFBOption == AUTO_FB_0) {
> - if (wRate < RATE_18M)
> - wRate = RATE_18M;
> - else if (wRate > RATE_54M)
> - wRate = RATE_54M;
> -
> - if (uFragIdx == (uMACfragNum - 2))
> - uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt0[FB_RATE0][wRate-RATE_18M], bNeedAck);
> - else
> - uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE0][wRate-RATE_18M], bNeedAck);
> -
> - } else { /* (byFBOption == AUTO_FB_1) */
> - if (wRate < RATE_18M)
> - wRate = RATE_18M;
> - else if (wRate > RATE_54M)
> - wRate = RATE_54M;
> -
> - if (uFragIdx == (uMACfragNum - 2))
> - uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt1[FB_RATE0][wRate-RATE_18M], bNeedAck);
> - else
> - uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt1[FB_RATE0][wRate-RATE_18M], bNeedAck);
> + uNextPktTime = s_uGetTxRsvTime(pDevice,
> + byPktType, len,
> + wFB_Opt0[FB_RATE0][wRate - RATE_18M],
> + bNeedAck);
> + } else {
> + uNextPktTime = s_uGetTxRsvTime(pDevice,
> + byPktType, len,
> + wFB_Opt1[FB_RATE0][wRate - RATE_18M],
> + bNeedAck);
> }
>
> if (bNeedAck) {
> - uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
> - return pDevice->uSIFS + uAckTime + uNextPktTime;
> + uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
> + byPktType, 14,
> + pDevice->byTopOFDMBasicRate);
> } else {
> - return pDevice->uSIFS + uNextPktTime;
> + uAckTime = 0;
> }
> +
> + return pDevice->uSIFS + uAckTime + uNextPktTime;
> }
> break;
>
> @@ -341,34 +345,28 @@ s_uGetDataDuration(
> return 0;
> }
> } else { /* First Frag or Mid Frag */
> +
> if (byFBOption == AUTO_FB_0) {
> - if (wRate < RATE_18M)
> - wRate = RATE_18M;
> - else if (wRate > RATE_54M)
> - wRate = RATE_54M;
> -
> - if (uFragIdx == (uMACfragNum - 2))
> - uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt0[FB_RATE1][wRate-RATE_18M], bNeedAck);
> - else
> - uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE1][wRate-RATE_18M], bNeedAck);
> -
> - } else { /* (byFBOption == AUTO_FB_1) */
> - if (wRate < RATE_18M)
> - wRate = RATE_18M;
> - else if (wRate > RATE_54M)
> - wRate = RATE_54M;
> -
> - if (uFragIdx == (uMACfragNum - 2))
> - uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt1[FB_RATE1][wRate-RATE_18M], bNeedAck);
> - else
> - uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt1[FB_RATE1][wRate-RATE_18M], bNeedAck);
> + uNextPktTime = s_uGetTxRsvTime(pDevice,
> + byPktType, len,
> + wFB_Opt0[FB_RATE0][wRate - RATE_18M],
> + bNeedAck);
> + } else {
> + uNextPktTime = s_uGetTxRsvTime(pDevice,
> + byPktType, len,
> + wFB_Opt1[FB_RATE0][wRate - RATE_18M],
> + bNeedAck);
> }
> +
> if (bNeedAck) {
> - uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
> - return pDevice->uSIFS + uAckTime + uNextPktTime;
> + uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
> + byPktType, 14,
> + pDevice->byTopOFDMBasicRate);
> } else {
> - return pDevice->uSIFS + uNextPktTime;
> + uAckTime = 0;
> }
> +
> + return pDevice->uSIFS + uAckTime + uNextPktTime;
> }
> break;
>
> --
> 2.24.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20200327213253.22230-1-briana.oursler%40gmail.com.
>
^ permalink raw reply [flat|nested] 15+ messages in thread* [patch v5] Staging: vt6655: Reduce statement complexity
2020-03-27 21:40 ` [Outreachy kernel] " Julia Lawall
@ 2020-03-28 5:10 ` Briana Oursler
2020-03-28 10:19 ` Julia Lawall
0 siblings, 1 reply; 15+ messages in thread
From: Briana Oursler @ 2020-03-28 5:10 UTC (permalink / raw)
To: julia.lawall, gregkh, nramas, forest, outreachy-kernel; +Cc: Briana Oursler
Move duplicated code out of nested if else statements and make other
simplifying changes, shortening line length and improving readability.
Signed-off-by: Briana Oursler <briana.oursler@gmail.com>
---
Changes in v5:
- Update patch description to better describe changes.
Changes in v4:
- Remove 'flag' bool and return comparison to switch case.
- Remove comment.
- Update path description to better describe changes.
Changes in v3:
- Update grammar and meaning in patch description.
Changes in v2:
- Remove ternary logic in switch case.
- Introduce 'flag' bool.
- Move len definition above switch case.
drivers/staging/vt6655/rxtx.c | 118 +++++++++++++++++-----------------
1 file changed, 58 insertions(+), 60 deletions(-)
diff --git a/drivers/staging/vt6655/rxtx.c b/drivers/staging/vt6655/rxtx.c
index 37fcc42ed000..9a6abe8613e6 100644
--- a/drivers/staging/vt6655/rxtx.c
+++ b/drivers/staging/vt6655/rxtx.c
@@ -240,10 +240,16 @@ s_uGetDataDuration(
{
bool bLastFrag = false;
unsigned int uAckTime = 0, uNextPktTime = 0;
+ unsigned int len;
if (uFragIdx == (uMACfragNum - 1))
bLastFrag = true;
+ if (uFragIdx == (uMACfragNum - 2))
+ len = cbLastFragmentSize;
+ else
+ len = cbFrameLength;
+
switch (byDurType) {
case DATADUR_B: /* DATADUR_B */
if (((uMACfragNum == 1)) || bLastFrag) {/* Non Frag or Last Frag */
@@ -254,17 +260,17 @@ 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);
+ 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);
} else {
- return pDevice->uSIFS + uNextPktTime;
+ uAckTime = 0;
}
+
+ return pDevice->uSIFS + uAckTime + uNextPktTime;
}
break;
@@ -277,17 +283,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);
+ uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, len,
+ wRate, bNeedAck);
if (bNeedAck) {
- uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
- return pDevice->uSIFS + uAckTime + uNextPktTime;
+ uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
+ byPktType, 14,
+ pDevice->byTopOFDMBasicRate);
} else {
- return pDevice->uSIFS + uNextPktTime;
+ uAckTime = 0;
}
+
+ return pDevice->uSIFS + uAckTime + uNextPktTime;
}
break;
@@ -300,35 +307,32 @@ s_uGetDataDuration(
return 0;
}
} else { /* First Frag or Mid Frag */
+ if (wRate < RATE_18M)
+ wRate = RATE_18M;
+ else if (wRate > RATE_54M)
+ wRate = RATE_54M;
+
if (byFBOption == AUTO_FB_0) {
- if (wRate < RATE_18M)
- wRate = RATE_18M;
- else if (wRate > RATE_54M)
- wRate = RATE_54M;
-
- if (uFragIdx == (uMACfragNum - 2))
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt0[FB_RATE0][wRate-RATE_18M], bNeedAck);
- else
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE0][wRate-RATE_18M], bNeedAck);
-
- } else { /* (byFBOption == AUTO_FB_1) */
- if (wRate < RATE_18M)
- wRate = RATE_18M;
- else if (wRate > RATE_54M)
- wRate = RATE_54M;
-
- if (uFragIdx == (uMACfragNum - 2))
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt1[FB_RATE0][wRate-RATE_18M], bNeedAck);
- else
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt1[FB_RATE0][wRate-RATE_18M], bNeedAck);
+ uNextPktTime = s_uGetTxRsvTime(pDevice,
+ byPktType, len,
+ wFB_Opt0[FB_RATE0][wRate - RATE_18M],
+ bNeedAck);
+ } else {
+ uNextPktTime = s_uGetTxRsvTime(pDevice,
+ byPktType, len,
+ wFB_Opt1[FB_RATE0][wRate - RATE_18M],
+ bNeedAck);
}
if (bNeedAck) {
- uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
- return pDevice->uSIFS + uAckTime + uNextPktTime;
+ uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
+ byPktType, 14,
+ pDevice->byTopOFDMBasicRate);
} else {
- return pDevice->uSIFS + uNextPktTime;
+ uAckTime = 0;
}
+
+ return pDevice->uSIFS + uAckTime + uNextPktTime;
}
break;
@@ -341,34 +345,28 @@ s_uGetDataDuration(
return 0;
}
} else { /* First Frag or Mid Frag */
+
if (byFBOption == AUTO_FB_0) {
- if (wRate < RATE_18M)
- wRate = RATE_18M;
- else if (wRate > RATE_54M)
- wRate = RATE_54M;
-
- if (uFragIdx == (uMACfragNum - 2))
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt0[FB_RATE1][wRate-RATE_18M], bNeedAck);
- else
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE1][wRate-RATE_18M], bNeedAck);
-
- } else { /* (byFBOption == AUTO_FB_1) */
- if (wRate < RATE_18M)
- wRate = RATE_18M;
- else if (wRate > RATE_54M)
- wRate = RATE_54M;
-
- if (uFragIdx == (uMACfragNum - 2))
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt1[FB_RATE1][wRate-RATE_18M], bNeedAck);
- else
- uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt1[FB_RATE1][wRate-RATE_18M], bNeedAck);
+ uNextPktTime = s_uGetTxRsvTime(pDevice,
+ byPktType, len,
+ wFB_Opt0[FB_RATE0][wRate - RATE_18M],
+ bNeedAck);
+ } else {
+ uNextPktTime = s_uGetTxRsvTime(pDevice,
+ byPktType, len,
+ wFB_Opt1[FB_RATE0][wRate - RATE_18M],
+ bNeedAck);
}
+
if (bNeedAck) {
- uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
- return pDevice->uSIFS + uAckTime + uNextPktTime;
+ uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
+ byPktType, 14,
+ pDevice->byTopOFDMBasicRate);
} else {
- return pDevice->uSIFS + uNextPktTime;
+ uAckTime = 0;
}
+
+ return pDevice->uSIFS + uAckTime + uNextPktTime;
}
break;
--
2.24.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [patch v5] Staging: vt6655: Reduce statement complexity
2020-03-28 5:10 ` [patch v5] Staging: vt6655: Reduce statement complexity Briana Oursler
@ 2020-03-28 10:19 ` Julia Lawall
2020-03-29 1:05 ` Briana Oursler
0 siblings, 1 reply; 15+ messages in thread
From: Julia Lawall @ 2020-03-28 10:19 UTC (permalink / raw)
To: Briana Oursler; +Cc: julia.lawall, gregkh, nramas, forest, outreachy-kernel
On Fri, 27 Mar 2020, Briana Oursler wrote:
> Move duplicated code out of nested if else statements and make other
> simplifying changes, shortening line length and improving readability.
Should this really be multiple patches? For example, does the len thing
need to be with the rest?
A line is added under an else in the hunk starting with line 341 that
doesn't look necessary.
julia
>
> Signed-off-by: Briana Oursler <briana.oursler@gmail.com>
> ---
>
> Changes in v5:
> - Update patch description to better describe changes.
>
> Changes in v4:
> - Remove 'flag' bool and return comparison to switch case.
> - Remove comment.
> - Update path description to better describe changes.
>
> Changes in v3:
> - Update grammar and meaning in patch description.
>
> Changes in v2:
> - Remove ternary logic in switch case.
> - Introduce 'flag' bool.
> - Move len definition above switch case.
> drivers/staging/vt6655/rxtx.c | 118 +++++++++++++++++-----------------
> 1 file changed, 58 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/staging/vt6655/rxtx.c b/drivers/staging/vt6655/rxtx.c
> index 37fcc42ed000..9a6abe8613e6 100644
> --- a/drivers/staging/vt6655/rxtx.c
> +++ b/drivers/staging/vt6655/rxtx.c
> @@ -240,10 +240,16 @@ s_uGetDataDuration(
> {
> bool bLastFrag = false;
> unsigned int uAckTime = 0, uNextPktTime = 0;
> + unsigned int len;
>
> if (uFragIdx == (uMACfragNum - 1))
> bLastFrag = true;
>
> + if (uFragIdx == (uMACfragNum - 2))
> + len = cbLastFragmentSize;
> + else
> + len = cbFrameLength;
> +
> switch (byDurType) {
> case DATADUR_B: /* DATADUR_B */
> if (((uMACfragNum == 1)) || bLastFrag) {/* Non Frag or Last Frag */
> @@ -254,17 +260,17 @@ 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);
> + 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);
> } else {
> - return pDevice->uSIFS + uNextPktTime;
> + uAckTime = 0;
> }
> +
> + return pDevice->uSIFS + uAckTime + uNextPktTime;
> }
> break;
>
> @@ -277,17 +283,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);
> + uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, len,
> + wRate, bNeedAck);
>
> if (bNeedAck) {
> - uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
> - return pDevice->uSIFS + uAckTime + uNextPktTime;
> + uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
> + byPktType, 14,
> + pDevice->byTopOFDMBasicRate);
> } else {
> - return pDevice->uSIFS + uNextPktTime;
> + uAckTime = 0;
> }
> +
> + return pDevice->uSIFS + uAckTime + uNextPktTime;
> }
> break;
>
> @@ -300,35 +307,32 @@ s_uGetDataDuration(
> return 0;
> }
> } else { /* First Frag or Mid Frag */
> + if (wRate < RATE_18M)
> + wRate = RATE_18M;
> + else if (wRate > RATE_54M)
> + wRate = RATE_54M;
> +
> if (byFBOption == AUTO_FB_0) {
> - if (wRate < RATE_18M)
> - wRate = RATE_18M;
> - else if (wRate > RATE_54M)
> - wRate = RATE_54M;
> -
> - if (uFragIdx == (uMACfragNum - 2))
> - uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt0[FB_RATE0][wRate-RATE_18M], bNeedAck);
> - else
> - uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE0][wRate-RATE_18M], bNeedAck);
> -
> - } else { /* (byFBOption == AUTO_FB_1) */
> - if (wRate < RATE_18M)
> - wRate = RATE_18M;
> - else if (wRate > RATE_54M)
> - wRate = RATE_54M;
> -
> - if (uFragIdx == (uMACfragNum - 2))
> - uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt1[FB_RATE0][wRate-RATE_18M], bNeedAck);
> - else
> - uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt1[FB_RATE0][wRate-RATE_18M], bNeedAck);
> + uNextPktTime = s_uGetTxRsvTime(pDevice,
> + byPktType, len,
> + wFB_Opt0[FB_RATE0][wRate - RATE_18M],
> + bNeedAck);
> + } else {
> + uNextPktTime = s_uGetTxRsvTime(pDevice,
> + byPktType, len,
> + wFB_Opt1[FB_RATE0][wRate - RATE_18M],
> + bNeedAck);
> }
>
> if (bNeedAck) {
> - uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
> - return pDevice->uSIFS + uAckTime + uNextPktTime;
> + uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
> + byPktType, 14,
> + pDevice->byTopOFDMBasicRate);
> } else {
> - return pDevice->uSIFS + uNextPktTime;
> + uAckTime = 0;
> }
> +
> + return pDevice->uSIFS + uAckTime + uNextPktTime;
> }
> break;
>
> @@ -341,34 +345,28 @@ s_uGetDataDuration(
> return 0;
> }
> } else { /* First Frag or Mid Frag */
> +
> if (byFBOption == AUTO_FB_0) {
> - if (wRate < RATE_18M)
> - wRate = RATE_18M;
> - else if (wRate > RATE_54M)
> - wRate = RATE_54M;
> -
> - if (uFragIdx == (uMACfragNum - 2))
> - uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt0[FB_RATE1][wRate-RATE_18M], bNeedAck);
> - else
> - uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE1][wRate-RATE_18M], bNeedAck);
> -
> - } else { /* (byFBOption == AUTO_FB_1) */
> - if (wRate < RATE_18M)
> - wRate = RATE_18M;
> - else if (wRate > RATE_54M)
> - wRate = RATE_54M;
> -
> - if (uFragIdx == (uMACfragNum - 2))
> - uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize, wFB_Opt1[FB_RATE1][wRate-RATE_18M], bNeedAck);
> - else
> - uNextPktTime = s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt1[FB_RATE1][wRate-RATE_18M], bNeedAck);
> + uNextPktTime = s_uGetTxRsvTime(pDevice,
> + byPktType, len,
> + wFB_Opt0[FB_RATE0][wRate - RATE_18M],
> + bNeedAck);
> + } else {
> + uNextPktTime = s_uGetTxRsvTime(pDevice,
> + byPktType, len,
> + wFB_Opt1[FB_RATE0][wRate - RATE_18M],
> + bNeedAck);
> }
> +
> if (bNeedAck) {
> - uAckTime = BBuGetFrameTime(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
> - return pDevice->uSIFS + uAckTime + uNextPktTime;
> + uAckTime = BBuGetFrameTime(pDevice->byPreambleType,
> + byPktType, 14,
> + pDevice->byTopOFDMBasicRate);
> } else {
> - return pDevice->uSIFS + uNextPktTime;
> + uAckTime = 0;
> }
> +
> + return pDevice->uSIFS + uAckTime + uNextPktTime;
> }
> break;
>
> --
> 2.24.1
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [patch v5] Staging: vt6655: Reduce statement complexity
2020-03-28 10:19 ` Julia Lawall
@ 2020-03-29 1:05 ` Briana Oursler
0 siblings, 0 replies; 15+ messages in thread
From: Briana Oursler @ 2020-03-29 1:05 UTC (permalink / raw)
To: Julia Lawall; +Cc: gregkh, nramas, forest, outreachy-kernel
On Sat, 2020-03-28 at 11:19 +0100, Julia Lawall wrote:
>
> On Fri, 27 Mar 2020, Briana Oursler wrote:
>
> > Move duplicated code out of nested if else statements and make
> > other
> > simplifying changes, shortening line length and improving
> > readability.
>
> Should this really be multiple patches? For example, does the len
> thing
> need to be with the rest?
>
> A line is added under an else in the hunk starting with line 341 that
> doesn't look necessary.
I see what you mean. Thanks for reviewing, I'll go ahead and resend as
a patch series.
Thanks,
--Briana
>
> julia
>
> > Signed-off-by: Briana Oursler <briana.oursler@gmail.com>
> > ---
> >
> > Changes in v5:
> > - Update patch description to better describe changes.
> >
> > Changes in v4:
> > - Remove 'flag' bool and return comparison to switch case.
> > - Remove comment.
> > - Update path description to better describe changes.
> >
> > Changes in v3:
> > - Update grammar and meaning in patch description.
> >
> > Changes in v2:
> > - Remove ternary logic in switch case.
> > - Introduce 'flag' bool.
> > - Move len definition above switch case.
> > drivers/staging/vt6655/rxtx.c | 118 +++++++++++++++++-------------
> > ----
> > 1 file changed, 58 insertions(+), 60 deletions(-)
> >
> > diff --git a/drivers/staging/vt6655/rxtx.c
> > b/drivers/staging/vt6655/rxtx.c
> > index 37fcc42ed000..9a6abe8613e6 100644
> > --- a/drivers/staging/vt6655/rxtx.c
> > +++ b/drivers/staging/vt6655/rxtx.c
> > @@ -240,10 +240,16 @@ s_uGetDataDuration(
> > {
> > bool bLastFrag = false;
> > unsigned int uAckTime = 0, uNextPktTime = 0;
> > + unsigned int len;
> >
> > if (uFragIdx == (uMACfragNum - 1))
> > bLastFrag = true;
> >
> > + if (uFragIdx == (uMACfragNum - 2))
> > + len = cbLastFragmentSize;
> > + else
> > + len = cbFrameLength;
> > +
> > switch (byDurType) {
> > case DATADUR_B: /* DATADUR_B */
> > if (((uMACfragNum == 1)) || bLastFrag) {/* Non Frag or
> > Last Frag */
> > @@ -254,17 +260,17 @@ 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);
> > + 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);
> > } else {
> > - return pDevice->uSIFS + uNextPktTime;
> > + uAckTime = 0;
> > }
> > +
> > + return pDevice->uSIFS + uAckTime +
> > uNextPktTime;
> > }
> > break;
> >
> > @@ -277,17 +283,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);
> > + uNextPktTime = s_uGetTxRsvTime(pDevice,
> > byPktType, len,
> > + wRate,
> > bNeedAck);
> >
> > if (bNeedAck) {
> > - uAckTime = BBuGetFrameTime(pDevice-
> > >byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
> > - return pDevice->uSIFS + uAckTime +
> > uNextPktTime;
> > + uAckTime = BBuGetFrameTime(pDevice-
> > >byPreambleType,
> > + byPktType,
> > 14,
> > + pDevice-
> > >byTopOFDMBasicRate);
> > } else {
> > - return pDevice->uSIFS + uNextPktTime;
> > + uAckTime = 0;
> > }
> > +
> > + return pDevice->uSIFS + uAckTime +
> > uNextPktTime;
> > }
> > break;
> >
> > @@ -300,35 +307,32 @@ s_uGetDataDuration(
> > return 0;
> > }
> > } else { /* First Frag or Mid Frag */
> > + if (wRate < RATE_18M)
> > + wRate = RATE_18M;
> > + else if (wRate > RATE_54M)
> > + wRate = RATE_54M;
> > +
> > if (byFBOption == AUTO_FB_0) {
> > - if (wRate < RATE_18M)
> > - wRate = RATE_18M;
> > - else if (wRate > RATE_54M)
> > - wRate = RATE_54M;
> > -
> > - if (uFragIdx == (uMACfragNum - 2))
> > - uNextPktTime =
> > s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize,
> > wFB_Opt0[FB_RATE0][wRate-RATE_18M], bNeedAck);
> > - else
> > - uNextPktTime =
> > s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength,
> > wFB_Opt0[FB_RATE0][wRate-RATE_18M], bNeedAck);
> > -
> > - } else { /* (byFBOption == AUTO_FB_1) */
> > - if (wRate < RATE_18M)
> > - wRate = RATE_18M;
> > - else if (wRate > RATE_54M)
> > - wRate = RATE_54M;
> > -
> > - if (uFragIdx == (uMACfragNum - 2))
> > - uNextPktTime =
> > s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize,
> > wFB_Opt1[FB_RATE0][wRate-RATE_18M], bNeedAck);
> > - else
> > - uNextPktTime =
> > s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength,
> > wFB_Opt1[FB_RATE0][wRate-RATE_18M], bNeedAck);
> > + uNextPktTime = s_uGetTxRsvTime(pDevice,
> > + byPktTyp
> > e, len,
> > + wFB_Opt0
> > [FB_RATE0][wRate - RATE_18M],
> > + bNeedAck
> > );
> > + } else {
> > + uNextPktTime = s_uGetTxRsvTime(pDevice,
> > + byPktTyp
> > e, len,
> > + wFB_Opt1
> > [FB_RATE0][wRate - RATE_18M],
> > + bNeedAck
> > );
> > }
> >
> > if (bNeedAck) {
> > - uAckTime = BBuGetFrameTime(pDevice-
> > >byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
> > - return pDevice->uSIFS + uAckTime +
> > uNextPktTime;
> > + uAckTime = BBuGetFrameTime(pDevice-
> > >byPreambleType,
> > + byPktType,
> > 14,
> > + pDevice-
> > >byTopOFDMBasicRate);
> > } else {
> > - return pDevice->uSIFS + uNextPktTime;
> > + uAckTime = 0;
> > }
> > +
> > + return pDevice->uSIFS + uAckTime +
> > uNextPktTime;
> > }
> > break;
> >
> > @@ -341,34 +345,28 @@ s_uGetDataDuration(
> > return 0;
> > }
> > } else { /* First Frag or Mid Frag */
> > +
> > if (byFBOption == AUTO_FB_0) {
> > - if (wRate < RATE_18M)
> > - wRate = RATE_18M;
> > - else if (wRate > RATE_54M)
> > - wRate = RATE_54M;
> > -
> > - if (uFragIdx == (uMACfragNum - 2))
> > - uNextPktTime =
> > s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize,
> > wFB_Opt0[FB_RATE1][wRate-RATE_18M], bNeedAck);
> > - else
> > - uNextPktTime =
> > s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength,
> > wFB_Opt0[FB_RATE1][wRate-RATE_18M], bNeedAck);
> > -
> > - } else { /* (byFBOption == AUTO_FB_1) */
> > - if (wRate < RATE_18M)
> > - wRate = RATE_18M;
> > - else if (wRate > RATE_54M)
> > - wRate = RATE_54M;
> > -
> > - if (uFragIdx == (uMACfragNum - 2))
> > - uNextPktTime =
> > s_uGetTxRsvTime(pDevice, byPktType, cbLastFragmentSize,
> > wFB_Opt1[FB_RATE1][wRate-RATE_18M], bNeedAck);
> > - else
> > - uNextPktTime =
> > s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength,
> > wFB_Opt1[FB_RATE1][wRate-RATE_18M], bNeedAck);
> > + uNextPktTime = s_uGetTxRsvTime(pDevice,
> > + byPktTyp
> > e, len,
> > + wFB_Opt0
> > [FB_RATE0][wRate - RATE_18M],
> > + bNeedAck
> > );
> > + } else {
> > + uNextPktTime = s_uGetTxRsvTime(pDevice,
> > + byPktTyp
> > e, len,
> > + wFB_Opt1
> > [FB_RATE0][wRate - RATE_18M],
> > + bNeedAck
> > );
> > }
> > +
> > if (bNeedAck) {
> > - uAckTime = BBuGetFrameTime(pDevice-
> > >byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
> > - return pDevice->uSIFS + uAckTime +
> > uNextPktTime;
> > + uAckTime = BBuGetFrameTime(pDevice-
> > >byPreambleType,
> > + byPktType,
> > 14,
> > + pDevice-
> > >byTopOFDMBasicRate);
> > } else {
> > - return pDevice->uSIFS + uNextPktTime;
> > + uAckTime = 0;
> > }
> > +
> > + return pDevice->uSIFS + uAckTime +
> > uNextPktTime;
> > }
> > break;
> >
> > --
> > 2.24.1
> >
> >
^ permalink raw reply [flat|nested] 15+ messages in thread