Skip to contentSkip to conversation

sound/soc/at32: Useless NULL test

PATCH In review Julien Brunel 3 participants 3 messages Sep 26, 2008

Subsystems (3)
Sound Kernel janitors ASoC
lore
Share & tools
lore

Julien Brunel

author
#0 lore raw

PATCH 1/1 sound/soc/at32: Useless NULL test

The test (ssc != NULL) can only be reached if the call to the function ssc_request, the result of which ssc is assigned, succeeds. Moreover, two statements assign NULL to ssc just before a return, which is useless since it is a local variable. So, we suggest to delete the test and the two assignments.

A simplified version of the semantic match that finds this problem is as follows:
(http://www.emn.fr/x-info/coccinelle/)

// <smpl>
@bad_null_test@
expression x,E;
@@
x = ssc_request(...)
... when != x = E
* x != NULL
// </smpl>

Signed-off-by Julien Brunel <[email protected]>
Signed-off-by Julia Lawall <[email protected]>
---
 sound/soc/at32/playpaq_wm8510.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
sound/soc/at32/playpaq_wm8510.c +1 -5
diff -u -p a/sound/soc/at32/playpaq_wm8510.c b/sound/soc/at32/playpaq_wm8510.c
@@ -405,7 +405,6 @@ static int __init playpaq_asoc_init(void
 	ssc = ssc_request(0);
 	if (IS_ERR(ssc)) {
 		ret = PTR_ERR(ssc);
-		ssc = NULL;
 		goto err_ssc;
 	}
 	ssc_p->ssc = ssc;

@@ -476,10 +475,7 @@ err_pll0:
 		_gclk0 = NULL;
 	}
 err_gclk0:
-	if (ssc != NULL) {
-		ssc_free(ssc);
-		ssc = NULL;
-	}
+	ssc_free(ssc);
 err_ssc:
 	return ret;
 }
Acked-by on v1 1/1
#1 lore raw
Quoted 5 lines · Julien Brunel

The test (ssc != NULL) can only be reached if the call to the function ssc_request, the result of which ssc is assigned, succeeds. Moreover, statements assign NULL to ssc just before a return, which is useless since it is a local variable. So, we suggest to delete the test and the two assignments.

Acked-by Mark Brown <[email protected]>

For future reference if you're submitting other similar things (which I guess you will given that you've got a tool to check for this stuff) it'd be helpful if you could rewrite the explanation for the NULL test to be something like:

The test (ssc != NULL) is redundant since it can only be reached when ssc is guaranteed to have been set to a valid ssc.

which is much easier to parse.

Quoted 20 lines

diff -u -p a/sound/soc/at32/playpaq_wm8510.c b/sound/soc/at32/playpaq_wm8510.c

sound/soc/at32/playpaq_wm8510.c +1 -5
@@ -405,7 +405,6 @@ static int __init playpaq_asoc_init(void
 	ssc = ssc_request(0);
 	if (IS_ERR(ssc)) {
 		ret = PTR_ERR(ssc);
-		ssc = NULL;
 		goto err_ssc;
 	}
 	ssc_p->ssc = ssc;

@@ -476,10 +475,7 @@ err_pll0:
 		_gclk0 = NULL;
 	}
 err_gclk0:
-	if (ssc != NULL) {
-		ssc_free(ssc);
-		ssc = NULL;
-	}
+	ssc_free(ssc);
#2 lore raw

At Fri, 26 Sep 2008 15:00:23 +0100,

Quoted 10 lines · Mark Brown
Quoted 5 lines · Julien Brunel

The test (ssc != NULL) can only be reached if the call to the function ssc_request, the result of which ssc is assigned, succeeds. Moreover, statements assign NULL to ssc just before a return, which is useless since it is a local variable. So, we suggest to delete the test and the two assignments.

Acked-by Mark Brown <[email protected]>

Applied now. Thanks.

Quoted 9 lines

For future reference if you're submitting other similar things (which I guess you will given that you've got a tool to check for this stuff) it'd be helpful if you could rewrite the explanation for the NULL test to be something like:

The test (ssc != NULL) is redundant since it can only be reached when ssc is guaranteed to have been set to a valid ssc.

which is much easier to parse.

Agreed :)

Takashi