diff mbox

[OPW,kernel] staging:rtl8188eu: Removed assignments in if statements in core/rtw_cmd.c. Fixed some indentation to silence checkpatch errors (albeit some warnings persist).

Message ID 1394262741-5682-1-git-send-email-fempsci@gmail.com
State New, archived
Headers show

Commit Message

Chi Pham March 8, 2014, 7:12 a.m. UTC
Signed-off-by: Chi Pham <fempsci@gmail.com>
---
 drivers/staging/rtl8188eu/core/rtw_cmd.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Sarah Sharp March 8, 2014, 8:04 p.m. UTC | #1
You've overflowed the subject line. :)

Please make sure you have a blank line between the patch subject and the
patch body.

On Sat, Mar 08, 2014 at 08:12:21AM +0100, Chi Pham wrote:
> Signed-off-by: Chi Pham <fempsci@gmail.com>
> ---
>  drivers/staging/rtl8188eu/core/rtw_cmd.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/rtl8188eu/core/rtw_cmd.c b/drivers/staging/rtl8188eu/core/rtw_cmd.c
> index 8c0d2eb..9e2ff7f 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_cmd.c
> @@ -1974,13 +1974,17 @@ static void c2h_wk_callback(struct work_struct *work)
>  	evtpriv->c2h_wk_alive = true;
>  
>  	while (!rtw_cbuf_empty(evtpriv->c2h_queue)) {
> -		if ((c2h_evt = (struct c2h_evt_hdr *)rtw_cbuf_pop(evtpriv->c2h_queue)) != NULL) {
> +		c2h_evt = (struct c2h_evt_hdr *)rtw_cbuf_pop(evtpriv->c2h_queue);
> +		if (c2h_evt != NULL) {

Might as well change that to
 +		if (c2h_evt) {

>  			/* This C2H event is read, clear it */
>  			c2h_evt_clear(adapter);
> -		} else if ((c2h_evt = (struct c2h_evt_hdr *)rtw_malloc(16)) != NULL) {
> +		} else {
> +			c2h_evt = (struct c2h_evt_hdr *)rtw_malloc(16);
> +			if (c2h_evt != NULL) {

Same here.

>  			/* This C2H event is not read, read & clear now */
> -			if (c2h_evt_read(adapter, (u8 *)c2h_evt) != _SUCCESS)
> -				continue;
> +				if (c2h_evt_read(adapter, (u8 *)c2h_evt) != _SUCCESS)
> +					continue;
> +			}
>  		}
>  
>  		/* Special pointer to trigger c2h_evt_clear only */
> -- 
> 1.7.9.5

Sarah Sharp
Chi Pham March 8, 2014, 8:49 p.m. UTC | #2
>
> Might as well change that to 
>  +                if (c2h_evt) { 
>
> I did notice, and I didn't change it because I thought that change didn't 
'belong' in this patch if that makes sense. 
So if I change it now, should I also mention it in the commit line (thereby 
overflowing it further :)?
Sarah Sharp March 8, 2014, 9:48 p.m. UTC | #3
On Sat, Mar 08, 2014 at 12:49:01PM -0800, Chi Pham wrote:
> 
> >
> > Might as well change that to 
> >  +                if (c2h_evt) { 
> >
> I did notice, and I didn't change it because I thought that change didn't 
> 'belong' in this patch if that makes sense. 

It's a judgment call as to whether those changes belong.

> So if I change it now, should I also mention it in the commit line (thereby 
> overflowing it further :)?

You should mention it in the commit message, and add a blank line so it
doesn't overflow. :)

Sarah Sharp
Chi Pham March 9, 2014, 10:01 a.m. UTC | #4
On Saturday, March 8, 2014 10:48:47 PM UTC+1, Sarah Sharp wrote:
>
> It's a judgment call as to whether those changes belong. 
>
> There's lots more of those in the file, so I'll make a separate patch for 
them.
 

> You should mention it in the commit message, and add a blank line so it 
> doesn't overflow. :) 
>

The commit message has a blank line between the message and the 
signed-off-by.
Or do you mean I should add a newline in the message itself?
Chi Pham March 9, 2014, 11:35 a.m. UTC | #5
On Sunday, March 9, 2014 11:01:21 AM UTC+1, Chi Pham wrote:
>
> On Saturday, March 8, 2014 10:48:47 PM UTC+1, Sarah Sharp wrote:
>>
>> It's a judgment call as to whether those changes belong. 
>>
>> There's lots more of those in the file, so I'll make a separate patch for 
> them.
>

It seems to be pretty prevalent just about everywhere in the drivers. 
Do you think it's maybe on purpose, for readability or the like?
Julia Lawall March 9, 2014, 12:35 p.m. UTC | #6
On Sun, 9 Mar 2014, Chi Pham wrote:

> On Saturday, March 8, 2014 10:48:47 PM UTC+1, Sarah Sharp wrote:
>       It's a judgment call as to whether those changes belong.
>
> There's lots more of those in the file, so I'll make a separate patch for
> them.
>  
>       You should mention it in the commit message, and add a blank
>       line so it
>       doesn't overflow. :)
>
>
> The commit message has a blank line between the message and the
> signed-off-by.
> Or do you mean I should add a newline in the message itself?

Your subject line is:

[PATCH] staging:rtl8188eu: Removed assignments in if statements in
core/rtw_cmd.c. Fixed some indentation to silence checkpatch errors
(albeit some warnings persist).

It should be something like

[PATCH] staging:rtl8188eu: Removed assignments in if statements

Then the commit message in the patch could be something like:

Removed assignments in if statements in core/rtw_cmd.c. Fixed some
indentation to silence checkpatch errors (albeit some warnings persist).

Perhaps it is not necessary to repeat "Removed assignments in if
statements...", because the subject line will also be part of the
permanent version of the patch.  But if you drop that part, you could say,
after a blank line,

Additionally, fixed some indentation to silence checkpatch errors (albeit
some warnings persist).

julia
Julia Lawall March 9, 2014, 12:45 p.m. UTC | #7
On Sun, 9 Mar 2014, Chi Pham wrote:

> On Sunday, March 9, 2014 11:01:21 AM UTC+1, Chi Pham wrote:
>       On Saturday, March 8, 2014 10:48:47 PM UTC+1, Sarah Sharp wrote:
>             It's a judgment call as to whether those changes
>             belong.
>
>       There's lots more of those in the file, so I'll make a separate
>       patch for them.
>
>
> It seems to be pretty prevalent just about everywhere in the drivers. 
> Do you think it's maybe on purpose, for readability or the like? 

Maybe different people have a notion of readability....  It is kind of a
tradeoff: !x is very short. x == NULL is very clear.  But

x = foo(...)
if (!x) { ... }

is a common pattern.

x = foo(...)
if (x == NULL) { ... }

looks a bit verbose.

julia
diff mbox

Patch

diff --git a/drivers/staging/rtl8188eu/core/rtw_cmd.c b/drivers/staging/rtl8188eu/core/rtw_cmd.c
index 8c0d2eb..9e2ff7f 100644
--- a/drivers/staging/rtl8188eu/core/rtw_cmd.c
+++ b/drivers/staging/rtl8188eu/core/rtw_cmd.c
@@ -1974,13 +1974,17 @@  static void c2h_wk_callback(struct work_struct *work)
 	evtpriv->c2h_wk_alive = true;
 
 	while (!rtw_cbuf_empty(evtpriv->c2h_queue)) {
-		if ((c2h_evt = (struct c2h_evt_hdr *)rtw_cbuf_pop(evtpriv->c2h_queue)) != NULL) {
+		c2h_evt = (struct c2h_evt_hdr *)rtw_cbuf_pop(evtpriv->c2h_queue);
+		if (c2h_evt != NULL) {
 			/* This C2H event is read, clear it */
 			c2h_evt_clear(adapter);
-		} else if ((c2h_evt = (struct c2h_evt_hdr *)rtw_malloc(16)) != NULL) {
+		} else {
+			c2h_evt = (struct c2h_evt_hdr *)rtw_malloc(16);
+			if (c2h_evt != NULL) {
 			/* This C2H event is not read, read & clear now */
-			if (c2h_evt_read(adapter, (u8 *)c2h_evt) != _SUCCESS)
-				continue;
+				if (c2h_evt_read(adapter, (u8 *)c2h_evt) != _SUCCESS)
+					continue;
+			}
 		}
 
 		/* Special pointer to trigger c2h_evt_clear only */