staging: r8188eu: fix suspect code indent for conditional statements

PATCH In review Rebecca Mckeever 3 participants 7 messages Mar 29, 2022 – Mar 31, 2022 Staging

Share & tools

Rebecca Mckeever

author

2022-03-29 07:53 UTC #0 lore raw

PATCH 1/1 staging: r8188eu: fix suspect code indent for conditional statements

Align the if and else if branches of the conditional statement to improve readability. Prevent bugs that could be introduced if developers misread the code. Issue found by checkpatch.

Signed-off-by Rebecca Mckeever <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/r8188eu/core/rtw_cmd.c +1 -1

index 6eca30124ee8..ccc43c0ba433 100644
@@ -1408,7 +1408,7 @@ void rtw_survey_cmd_callback(struct adapter *padapter,  struct cmd_obj *pcmd)
 		/* TODO: cancel timer and do timeout handler directly... */
 		/* need to make timeout handlerOS independent */
 		_set_timer(&pmlmepriv->scan_to_timer, 1);
-		} else if (pcmd->res != H2C_SUCCESS) {
+	} else if (pcmd->res != H2C_SUCCESS) {
 		_set_timer(&pmlmepriv->scan_to_timer, 1);
 	}

Signature · 1 line

2.32.0

Alison Schofield

2022-03-29 16:15 UTC #1 lore raw

Quoted 3 lines · Rebecca Mckeever

Align the if and else if branches of the conditional statement to improve readability. Prevent bugs that could be introduced if developers misread the code. Issue found by checkpatch.

Thanks for the patch Rebecca!

Lots of stuff done right - passes chkp, compiles, patch is sent to correct recipients, the commit message follows the format of the file.

Let's set a pattern here for all checkpatch related cleanups, for you and others that follow.(Thanks for being the first ;))

Commit msg says 'what'. Commit log says 'why'. Acknowledge that it was found using checkpatch in the commit log also. (In the future you may be acknowledging use of other tools like sparse, coccinelle.)

Note that the 'why' is never that a tool reported an error. The 'why' for these checkpatch reports is usually to follow the Linux Kernel Coding Style.

'Fix' in the commit message is needlessly generic. Perhaps:
[PATCH] staging: r8188eu: align both branches of a conditional statement

Commit log: (what you have is fine in the log)
I usually paste in the checkpatch error explicitly so it can be grep'd for. Something like:

Issue found by checkpatch:
WARNING: suspect code indent for conditional statements

Thanks,
Alison

Quoted 23 lines

Signed-off-by Rebecca Mckeever <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/r8188eu/core/rtw_cmd.c +1 -1

index 6eca30124ee8..ccc43c0ba433 100644
@@ -1408,7 +1408,7 @@ void rtw_survey_cmd_callback(struct adapter *padapter,  struct cmd_obj *pcmd)
 		/* TODO: cancel timer and do timeout handler directly... */
 		/* need to make timeout handlerOS independent */
 		_set_timer(&pmlmepriv->scan_to_timer, 1);
-		} else if (pcmd->res != H2C_SUCCESS) {
+	} else if (pcmd->res != H2C_SUCCESS) {
 		_set_timer(&pmlmepriv->scan_to_timer, 1);
 	}

Signature · 1 line

2.32.0

Rebecca Mckeever

author

2022-03-30 00:48 UTC #2 lore raw

Quoted 24 lines · Alison Schofield

Quoted 3 lines · Rebecca Mckeever
Align the if and else if branches of the conditional statement to improve readability. Prevent bugs that could be introduced if developers misread the code. Issue found by checkpatch.
Thanks for the patch Rebecca!
Lots of stuff done right - passes chkp, compiles, patch is sent to correct recipients, the commit message follows the format of the file.
Let's set a pattern here for all checkpatch related cleanups, for you and others that follow.(Thanks for being the first ;))
Commit msg says 'what'. Commit log says 'why'. Acknowledge that it was found using checkpatch in the commit log also. (In the future you may be acknowledging use of other tools like sparse, coccinelle.)
Note that the 'why' is never that a tool reported an error. The 'why' for these checkpatch reports is usually to follow the Linux Kernel Coding Style.
'Fix' in the commit message is needlessly generic. Perhaps:
[PATCH] staging: r8188eu: align both branches of a conditional statement

Quoted 6 lines

Commit log: (what you have is fine in the log)
I usually paste in the checkpatch error explicitly so it can be grep'd for. Something like:
Issue found by checkpatch:
WARNING: suspect code indent for conditional statements

There was a section of https://kernelnewbies.org/PatchPhilosophy that suggested putting the warning message in the subject line. I thought it would be redundant to also put it in the body. Is it a good practice to include the warning message in both places?

Quoted 23 lines

Thanks,
Alison

Quoted 19 lines

Signed-off-by Rebecca Mckeever <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/r8188eu/core/rtw_cmd.c +1 -1

index 6eca30124ee8..ccc43c0ba433 100644
@@ -1408,7 +1408,7 @@ void rtw_survey_cmd_callback(struct adapter *padapter,  struct cmd_obj *pcmd)
 		/* TODO: cancel timer and do timeout handler directly... */
 		/* need to make timeout handlerOS independent */
 		_set_timer(&pmlmepriv->scan_to_timer, 1);
-		} else if (pcmd->res != H2C_SUCCESS) {
+	} else if (pcmd->res != H2C_SUCCESS) {
 		_set_timer(&pmlmepriv->scan_to_timer, 1);
 	}

Quoted 5 lines

Quoted 4 lines
Signature · 1 line
2.32.0

Thanks,
Rebecca

Alison Schofield

2022-03-30 01:07 UTC #3 lore raw

Quoted 29 lines · Rebecca Mckeever

Quoted 28 lines · Alison Schofield
Quoted 3 lines · Rebecca Mckeever
Align the if and else if branches of the conditional statement to improve readability. Prevent bugs that could be introduced if developers misread the code. Issue found by checkpatch.
Thanks for the patch Rebecca!
Lots of stuff done right - passes chkp, compiles, patch is sent to correct recipients, the commit message follows the format of the file.
Let's set a pattern here for all checkpatch related cleanups, for you and others that follow.(Thanks for being the first ;))
Commit msg says 'what'. Commit log says 'why'. Acknowledge that it was found using checkpatch in the commit log also. (In the future you may be acknowledging use of other tools like sparse, coccinelle.)
Note that the 'why' is never that a tool reported an error. The 'why' for these checkpatch reports is usually to follow the Linux Kernel Coding Style.
'Fix' in the commit message is needlessly generic. Perhaps:
[PATCH] staging: r8188eu: align both branches of a conditional statement
Commit log: (what you have is fine in the log)
I usually paste in the checkpatch error explicitly so it can be grep'd for. Something like:

Quoted 8 lines

Quoted 2 lines
Issue found by checkpatch:
WARNING: suspect code indent for conditional statements
There was a section of https://kernelnewbies.org/PatchPhilosophy that suggested putting the warning message in the subject line. I thought it would be redundant to also put it in the body. Is it a good practice to include the warning message in both places?

The commit msg should say what you did. If it happens to match the warning message, that's fine. Think of the commit msg as directives to a machine.

This patch has already been applied :)
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=20b4b3fb383b3a499b8b47daaf1d6325faa9cfe2

You can view contents of staging-testing here:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/log/?h=staging-testing

Quoted 31 lines

Quoted 23 lines

Thanks,
Alison

Quoted 19 lines

Signed-off-by Rebecca Mckeever <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/r8188eu/core/rtw_cmd.c +1 -1

index 6eca30124ee8..ccc43c0ba433 100644
@@ -1408,7 +1408,7 @@ void rtw_survey_cmd_callback(struct adapter *padapter,  struct cmd_obj *pcmd)
 		/* TODO: cancel timer and do timeout handler directly... */
 		/* need to make timeout handlerOS independent */
 		_set_timer(&pmlmepriv->scan_to_timer, 1);
-		} else if (pcmd->res != H2C_SUCCESS) {
+	} else if (pcmd->res != H2C_SUCCESS) {
 		_set_timer(&pmlmepriv->scan_to_timer, 1);
 	}

Quoted 5 lines

Quoted 4 lines
Signature · 1 line
2.32.0

Thanks,
Rebecca

Dan Carpenter

2022-03-30 06:28 UTC #4 lore raw

In reply to Alison Schofield

I don't really like the focus on commit message minutia... :/ Basically everyone can understand the commit message. There are one or two maintainers who will fly into a rage when they see the word "fix" in a commit message but I have a simple solution where I just never email them again. My time is too valuable for that nonsense.

We would have applied this patch as is. Or I would normally have written it like this:

[PATCH] staging: r8188eu: Delete a stray tab in rtw_survey_cmd_callback()

This code works fine, but the line is indented too far so it's confusing. Delete a tab.

Signed-off-by ...

I had reviewed this patch earlier and almost pointed out that both sides of the if statement are the same except for the comment. The "need to make timeout handlerOS independent" comment is wrong. I have not looked at the details of the other comment.

I did not send my review comments because the patch was fine. But what we want is for the code to look more like this.

regards,
dan carpenter

drivers/staging/r8188eu/core/rtw_cmd.c +1 -4

index 6eca30124ee8..dcf7b24f95a8 100644
@@ -1404,11 +1404,8 @@ void rtw_survey_cmd_callback(struct adapter *padapter,  struct cmd_obj *pcmd)
 {
 	struct	mlme_priv *pmlmepriv = &padapter->mlmepriv;
 
-	if (pcmd->res == H2C_DROPPED) {
+	if (pcmd->res != H2C_SUCCESS) {
 		/* TODO: cancel timer and do timeout handler directly... */
-		/* need to make timeout handlerOS independent */
-		_set_timer(&pmlmepriv->scan_to_timer, 1);
-		} else if (pcmd->res != H2C_SUCCESS) {
 		_set_timer(&pmlmepriv->scan_to_timer, 1);
 	}

Rebecca Mckeever

author

2022-03-31 12:06 UTC #5 lore raw

Hi Dan,

Quoted 34 lines · Dan Carpenter

I don't really like the focus on commit message minutia... :/ Basically everyone can understand the commit message. There are one or two maintainers who will fly into a rage when they see the word "fix" in a commit message but I have a simple solution where I just never email them again. My time is too valuable for that nonsense.
We would have applied this patch as is. Or I would normally have written it like this:
[PATCH] staging: r8188eu: Delete a stray tab in rtw_survey_cmd_callback()
This code works fine, but the line is indented too far so it's confusing. Delete a tab.
Signed-off-by ...
I had reviewed this patch earlier and almost pointed out that both sides of the if statement are the same except for the comment. The "need to make timeout handlerOS independent" comment is wrong. I have not looked at the details of the other comment.
I did not send my review comments because the patch was fine. But what we want is for the code to look more like this.
regards,
dan carpenter
drivers/staging/r8188eu/core/rtw_cmd.c +0 -0
index 6eca30124ee8..dcf7b24f95a8 100644
@@ -1404,11 +1404,8 @@ void rtw_survey_cmd_callback(struct adapter *padapter,  struct cmd_obj *pcmd)
 {
 	struct	mlme_priv *pmlmepriv = &padapter->mlmepriv;

drivers/staging/r8188eu/core/rtw_cmd.c void rtw_survey_cmd_callback(struct adapter *padapter, struct cmd_obj *pcmd) · line 1411

Quoted 10 lines

drivers/staging/r8188eu/core/rtw_cmd.c +1 -4

 
-	if (pcmd->res == H2C_DROPPED) {
+	if (pcmd->res != H2C_SUCCESS) {
 		/* TODO: cancel timer and do timeout handler directly... */
-		/* need to make timeout handlerOS independent */
-		_set_timer(&pmlmepriv->scan_to_timer, 1);
-		} else if (pcmd->res != H2C_SUCCESS) {
 		_set_timer(&pmlmepriv->scan_to_timer, 1);
 	}

Is it okay if I submit a patch to implement your suggestion? I would include a "Suggested-by" tag.

Thanks,
Rebecca

Dan Carpenter

2022-03-31 12:22 UTC #6 lore raw

Quoted 8 lines · Rebecca Mckeever

Quoted 8 lines

drivers/staging/r8188eu/core/rtw_cmd.c +0 -0

index 6eca30124ee8..dcf7b24f95a8 100644
@@ -1404,11 +1404,8 @@ void rtw_survey_cmd_callback(struct adapter *padapter,  struct cmd_obj *pcmd)
 {
 	struct	mlme_priv *pmlmepriv = &padapter->mlmepriv;

Quoted 12 lines

drivers/staging/r8188eu/core/rtw_cmd.c void rtw_survey_cmd_callback(struct adapter *padapter, struct cmd_obj *pcmd) · line 1411
Quoted 9 lines
drivers/staging/r8188eu/core/rtw_cmd.c +1 -4
-	if (pcmd->res == H2C_DROPPED) {
+	if (pcmd->res != H2C_SUCCESS) {
 		/* TODO: cancel timer and do timeout handler directly... */
-		/* need to make timeout handlerOS independent */
-		_set_timer(&pmlmepriv->scan_to_timer, 1);
-		} else if (pcmd->res != H2C_SUCCESS) {
 		_set_timer(&pmlmepriv->scan_to_timer, 1);
 	}
 
Is it okay if I submit a patch to implement your suggestion? I would include a "Suggested-by" tag.

Yes, that's always okay.

I would probably send it as one patch, but I suggest you would be better off sending it as two uncontroversial patches. patch 1: delete the "handlerOS independent" comment. Patch2: combine both sides of the if statement.

regards,
dan carpenter