This patch fixes the checkpatch warning that else is not generally
useful after a break or return.
This was done using Coccinelle:
@@
expression e2;
statement s1;
@@
if(e2) { ... return ...; }
-else
s1
Signed-off-by: simran singhal <redacted>
---
drivers/staging/sm750fb/ddk750_sii164.c | 6 ++----
drivers/staging/sm750fb/ddk750_swi2c.c | 6 ++----
2 files changed, 4 insertions(+), 8 deletions(-)
This patch fixes the checkpatch warning that else is not generally
useful after a break or return.
This was done using Coccinelle:
@@
expression e2;
statement s1;
@@
if(e2) { ... return ...; }
-else
s1
Signed-off-by: simran singhal <redacted>
---
drivers/staging/gdm724x/gdm_endian.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
This patch fixes the checkpatch warning that else is not generally
useful after a break or return.
This was done using Coccinelle:
@@
expression e2;
statement s1;
@@
if(e2) { ... return ...; }
-else
s1
Signed-off-by: simran singhal <redacted>
---
drivers/staging/rtl8712/os_intfs.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
This patch fixes the checkpatch warning that else is not generally
useful after a break or return.
This was done using Coccinelle:
@@
expression e2;
statement s1;
@@
if(e2) { ... return ...; }
-else
s1
Signed-off-by: simran singhal <redacted>
---
drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
@@ -295,8 +294,7 @@ static long sw_i2c_write_byte(unsigned char data) if (i < 0xff) return 0;- else- return -1;+ return -1;
Assuming -1 is some sort of error,
it'd be a more common style to use
if (i >= 0xff)
return -1;
return 0;
Looking at the code, it might make
sense to use something like:
/* SDA still != 0 */
if (i >= 0xff)
return -1;
return 0;
}
@@ -295,8 +294,7 @@ static long sw_i2c_write_byte(unsigned char data) if (i < 0xff) return 0;- else- return -1;+ return -1;
Assuming -1 is some sort of error,
it'd be a more common style to use
if (i >= 0xff)
return -1;
return 0;
Looking at the code, it might make
sense to use something like:
/* SDA still != 0 */
if (i >= 0xff)
return -1;
return 0;
}
From: Julia Lawall <hidden> Date: 2017-02-27 21:27:59
On Mon, 27 Feb 2017, simran singhal wrote:
quoted hunk
This patch fixes the checkpatch warning that else is not generally
useful after a break or return.
This was done using Coccinelle:
@@
expression e2;
statement s1;
@@
if(e2) { ... return ...; }
-else
s1
Signed-off-by: simran singhal <redacted>
---
drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_tkip.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
@@ -374,8 +374,7 @@ static int ieee80211_tkip_encrypt(struct sk_buff *skb, int hdr_len, void *priv)if(!tcb_desc->bHwSec)returnret;-else-return0;+return0;
In contrast to another patch I commented on, it seems likely that here 0
means success. Converting 0 to false when that is what it means (ie not
here) makes the code more understandable.
julia
From: Julia Lawall <hidden> Date: 2017-02-27 22:39:58
On Mon, 27 Feb 2017, simran singhal wrote:
quoted hunk
This patch fixes the checkpatch warning that else is not generally
useful after a break or return.
This was done using Coccinelle:
@@
expression e2;
statement s1;
@@
if(e2) { ... return ...; }
-else
s1
Signed-off-by: simran singhal <redacted>
---
drivers/staging/sm750fb/ddk750_sii164.c | 6 ++----
drivers/staging/sm750fb/ddk750_swi2c.c | 6 ++----
2 files changed, 4 insertions(+), 8 deletions(-)
Totally unrelated to what you are doing, but I wonder if these return
values should be true and false? Perhaps it would help to see how the
return values are used at the calling context.
julia
From: Julia Lawall <hidden> Date: 2017-02-28 01:03:36
On Mon, 27 Feb 2017, simran singhal wrote:
This patch fixes the checkpatch warning that else is not generally
useful after a break or return.
This was done using Coccinelle:
@@
expression e2;
statement s1;
@@
if(e2) { ... return ...; }
-else
s1
One might be surprised that the following code was detected using the
above semantic patch, because in the code below there is no return in the
if branches. Actually, as a special feature, when one has an if branch
that ends in return, Coccinelle will skip through any gotos and see if the
return is matched afterward. Indeed it is a common pattern to have
if (...) {
foo(x);
bar(y);
return -ENOMEM;
}
But the code can also be cut up as eg
if (...) {
ret = -ENOMEM;
goto out;
}
...
out:
foo(x);
bar(y);
return ret;
To avoid having to write multiple patterns for these cases, Coccinelle
will just jump through the return in the second case, allowing the same
pattern to match both of them.
julia
On Wed, Mar 1, 2017 at 12:30 AM, Julia Lawall [off-list ref] wrote:
On Wed, 1 Mar 2017, SIMRAN SINGHAL wrote:
quoted
On Tue, Feb 28, 2017 at 2:45 AM, Julia Lawall [off-list ref] wrote:
quoted
On Mon, 27 Feb 2017, simran singhal wrote:
quoted
This patch fixes the checkpatch warning that else is not generally
useful after a break or return.
This was done using Coccinelle:
@@
expression e2;
statement s1;
@@
if(e2) { ... return ...; }
-else
s1
Signed-off-by: simran singhal <redacted>
---
drivers/staging/sm750fb/ddk750_sii164.c | 6 ++----
drivers/staging/sm750fb/ddk750_swi2c.c | 6 ++----
2 files changed, 4 insertions(+), 8 deletions(-)
Totally unrelated to what you are doing, but I wonder if these return
values should be true and false? Perhaps it would help to see how the
return values are used at the calling context.
you want me to go ahead with what Joe mentioned and also do the same
changes here also.
I think that it would be best to complete what you have underway already.
When those patches are picked up by Greg, you can look into the
possibility of using true and false.
There might be a case for this one.
error returns are generally in the form
{
[...]
err = func(...);
if (err < 0)
return err;
return 0;
}
Not sure, what's the problem in removing else as according to me
there is no use of else.
In this case if (if condition) does not satisfy then else condition will
be satisfied and function will return 0.
From: Julia Lawall <hidden> Date: 2017-02-28 19:35:55
On Wed, 1 Mar 2017, SIMRAN SINGHAL wrote:
On Tue, Feb 28, 2017 at 2:49 AM, Julia Lawall [off-list ref] wrote:
quoted
On Mon, 27 Feb 2017, simran singhal wrote:
quoted
This patch fixes the checkpatch warning that else is not generally
useful after a break or return.
This was done using Coccinelle:
@@
expression e2;
statement s1;
@@
if(e2) { ... return ...; }
-else
s1
One might be surprised that the following code was detected using the
above semantic patch, because in the code below there is no return in the
if branches. Actually, as a special feature, when one has an if branch
that ends in return, Coccinelle will skip through any gotos and see if the
return is matched afterward. Indeed it is a common pattern to have
if (...) {
foo(x);
bar(y);
return -ENOMEM;
}
But the code can also be cut up as eg
if (...) {
ret = -ENOMEM;
goto out;
}
...
out:
foo(x);
bar(y);
return ret;
To avoid having to write multiple patterns for these cases, Coccinelle
will just jump through the return in the second case, allowing the same
pattern to match both of them.
Julia, Thanks for explaination. Its really helpful.
But I think there is no problem in removing else.
Because it will execute this
padapter->dvobjpriv.inirp_init(padapter);
when if condition will not satisfy.
From: Joe Perches <joe@perches.com> Date: 2017-02-28 19:47:03
On Wed, 2017-03-01 at 00:41 +0530, SIMRAN SINGHAL wrote:
On Tue, Feb 28, 2017 at 1:49 AM, SIMRAN SINGHAL
[off-list ref] wrote:
quoted
On Tue, Feb 28, 2017 at 1:11 AM, Joe Perches [off-list ref] wrote:
quoted
On Mon, 2017-02-27 at 23:44 +0530, simran singhal wrote:
quoted
This patch fixes the checkpatch warning that else is not generally
useful after a break or return.
This was done using Coccinelle:
@@
expression e2;
statement s1;
@@
if(e2) { ... return ...; }
-else
s1
again, not a checkpatch message for any of the
suggested modified hunks.
I am not getting what's the problem in removing else or may be I
am wrong you just want to say that I should change the commit message.
2 things:
1: The commit message is incorrect.
2: This form is fundamentally OK:
if (foo)
return bar;
else
return baz;
So I think this patch is not good.
again, not a checkpatch message for any of the
suggested modified hunks.
I am not getting what's the problem in removing else or may be I
am wrong you just want to say that I should change the commit message.
Yes, I think that the issue is just the commit message. Was it really
checkpatch that motivated you to do this? Joe maintains checkpatch, and
he doesn't think that it gives such a warning.
julia
From: Julia Lawall <hidden> Date: 2017-02-28 20:01:36
On Tue, 28 Feb 2017, Joe Perches wrote:
On Wed, 2017-03-01 at 00:41 +0530, SIMRAN SINGHAL wrote:
quoted
On Tue, Feb 28, 2017 at 1:49 AM, SIMRAN SINGHAL
[off-list ref] wrote:
quoted
On Tue, Feb 28, 2017 at 1:11 AM, Joe Perches [off-list ref] wrote:
quoted
On Mon, 2017-02-27 at 23:44 +0530, simran singhal wrote:
quoted
This patch fixes the checkpatch warning that else is not generally
useful after a break or return.
This was done using Coccinelle:
@@
expression e2;
statement s1;
@@
if(e2) { ... return ...; }
-else
s1
again, not a checkpatch message for any of the
suggested modified hunks.
I am not getting what's the problem in removing else or may be I
am wrong you just want to say that I should change the commit message.
2 things:
1: The commit message is incorrect.
2: This form is fundamentally OK:
if (foo)
return bar;
else
return baz;
So I think this patch is not good.
I agree in this case. The two branches are quite parallel. In some of
the other patches, the if was looking for the absence of some resource or
the failure of something, so there was a clear distinction between one
branch being cleanup on failure and the other branch being the continuing
successful computation, even if it is just to return a success indicator.
julia
On Tue, Feb 28, 2017 at 2:49 AM, Julia Lawall [off-list ref] wrote:
On Mon, 27 Feb 2017, simran singhal wrote:
quoted
This patch fixes the checkpatch warning that else is not generally
useful after a break or return.
This was done using Coccinelle:
@@
expression e2;
statement s1;
@@
if(e2) { ... return ...; }
-else
s1
One might be surprised that the following code was detected using the
above semantic patch, because in the code below there is no return in the
if branches. Actually, as a special feature, when one has an if branch
that ends in return, Coccinelle will skip through any gotos and see if the
return is matched afterward. Indeed it is a common pattern to have
if (...) {
foo(x);
bar(y);
return -ENOMEM;
}
But the code can also be cut up as eg
if (...) {
ret = -ENOMEM;
goto out;
}
...
out:
foo(x);
bar(y);
return ret;
To avoid having to write multiple patterns for these cases, Coccinelle
will just jump through the return in the second case, allowing the same
pattern to match both of them.
Julia, Thanks for explaination. Its really helpful.
But I think there is no problem in removing else.
Because it will execute this
padapter->dvobjpriv.inirp_init(padapter);
when if condition will not satisfy.
On Tue, Feb 28, 2017 at 2:45 AM, Julia Lawall [off-list ref] wrote:
On Mon, 27 Feb 2017, simran singhal wrote:
quoted
This patch fixes the checkpatch warning that else is not generally
useful after a break or return.
This was done using Coccinelle:
@@
expression e2;
statement s1;
@@
if(e2) { ... return ...; }
-else
s1
Signed-off-by: simran singhal <redacted>
---
drivers/staging/sm750fb/ddk750_sii164.c | 6 ++----
drivers/staging/sm750fb/ddk750_swi2c.c | 6 ++----
2 files changed, 4 insertions(+), 8 deletions(-)
Totally unrelated to what you are doing, but I wonder if these return
values should be true and false? Perhaps it would help to see how the
return values are used at the calling context.
you want me to go ahead with what Joe mentioned and also do the same
changes here also.
From: Julia Lawall <hidden> Date: 2017-03-01 00:43:16
On Wed, 1 Mar 2017, SIMRAN SINGHAL wrote:
On Tue, Feb 28, 2017 at 2:45 AM, Julia Lawall [off-list ref] wrote:
quoted
On Mon, 27 Feb 2017, simran singhal wrote:
quoted
This patch fixes the checkpatch warning that else is not generally
useful after a break or return.
This was done using Coccinelle:
@@
expression e2;
statement s1;
@@
if(e2) { ... return ...; }
-else
s1
Signed-off-by: simran singhal <redacted>
---
drivers/staging/sm750fb/ddk750_sii164.c | 6 ++----
drivers/staging/sm750fb/ddk750_swi2c.c | 6 ++----
2 files changed, 4 insertions(+), 8 deletions(-)
Totally unrelated to what you are doing, but I wonder if these return
values should be true and false? Perhaps it would help to see how the
return values are used at the calling context.
you want me to go ahead with what Joe mentioned and also do the same
changes here also.
I think that it would be best to complete what you have underway already.
When those patches are picked up by Greg, you can look into the
possibility of using true and false.
julia
There might be a case for this one.
error returns are generally in the form
{
[...]
err = func(...);
if (err < 0)
return err;
return 0;
}
Not sure, what's the problem in removing else as according to me
there is no use of else.
In this case if (if condition) does not satisfy then else condition will
be satisfied and function will return 0.
I think that "there might be a case for" was a positive comment. In any
case, it looks nicer to me if success is outside of a conditional and
failure is under a conditional. Or to be more precise, Coccinelle bug
finding rules tend to work better if that property is respected.
julia