[PATCH] staging: goldfish: Fix minor coding style

[PATCH] staging: goldfish: Fix minor coding style

[Loic Pefferkorn]

Hello,

The convention for checking for NULL pointers is !ptr and not ptr == NULL.
This patch fixes such occurences in goldfish driver, it applies against next-20141212


Signed-off-by: Loic Pefferkorn <loic@loicp.eu>
---
 drivers/staging/goldfish/goldfish_audio.c |  8 ++++----
 drivers/staging/goldfish/goldfish_nand.c  | 10 +++++-----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/goldfish/goldfish_audio.c b/drivers/staging/goldfish/goldfish_audio.c
index f200359..7ab034b 100644
--- a/drivers/staging/goldfish/goldfish_audio.c
+++ b/drivers/staging/goldfish/goldfish_audio.c
@@ -273,19 +273,19 @@ static int goldfish_audio_probe(struct platform_device *pdev)
 	dma_addr_t buf_addr;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
-	if (data == NULL)
+	if (!data)
 		return -ENOMEM;
 	spin_lock_init(&data->lock);
 	init_waitqueue_head(&data->wait);
 	platform_set_drvdata(pdev, data);
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (r == NULL) {
+	if (!r) {
 		dev_err(&pdev->dev, "platform_get_resource failed\n");
 		return -ENODEV;
 	}
 	data->reg_base = devm_ioremap(&pdev->dev, r->start, PAGE_SIZE);
-	if (data->reg_base == NULL)
+	if (!data->reg_base)
 		return -ENOMEM;
 
 	data->irq = platform_get_irq(pdev, 0);
@@ -295,7 +295,7 @@ static int goldfish_audio_probe(struct platform_device *pdev)
 	}
 	data->buffer_virt = dmam_alloc_coherent(&pdev->dev,
 				COMBINED_BUFFER_SIZE, &buf_addr, GFP_KERNEL);
-	if (data->buffer_virt == NULL) {
+	if (!data->buffer_virt) {
 		dev_err(&pdev->dev, "allocate buffer failed\n");
 		return -ENOMEM;
 	}
diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c
index d68f216..8e8e594 100644
--- a/drivers/staging/goldfish/goldfish_nand.c
+++ b/drivers/staging/goldfish/goldfish_nand.c
@@ -48,7 +48,7 @@ static u32 goldfish_nand_cmd_with_params(struct mtd_info *mtd,
 	struct cmd_params *cps = nand->cmd_params;
 	unsigned char __iomem  *base = nand->base;
 
-	if (cps == NULL)
+	if (!cps)
 		return -1;
 
 	switch (cmd) {
@@ -330,7 +330,7 @@ static int goldfish_nand_init_device(struct platform_device *pdev,
 	mtd->priv = nand;
 
 	name = devm_kzalloc(&pdev->dev, name_len + 1, GFP_KERNEL);
-	if (name == NULL)
+	if (!name)
 		return -ENOMEM;
 	mtd->name = name;
 
@@ -379,11 +379,11 @@ static int goldfish_nand_probe(struct platform_device *pdev)
 	unsigned char __iomem  *base;
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (r == NULL)
+	if (!r)
 		return -ENODEV;
 
 	base = devm_ioremap(&pdev->dev, r->start, PAGE_SIZE);
-	if (base == NULL)
+	if (!base)
 		return -ENOMEM;
 
 	version = readl(base + NAND_VERSION);
@@ -399,7 +399,7 @@ static int goldfish_nand_probe(struct platform_device *pdev)
 
 	nand = devm_kzalloc(&pdev->dev, sizeof(*nand) +
 				sizeof(struct mtd_info) * num_dev, GFP_KERNEL);
-	if (nand == NULL)
+	if (!nand)
 		return -ENOMEM;
 
 	mutex_init(&nand->lock);
-- 
2.1.3

Re: [PATCH] staging: goldfish: Fix minor coding style

[Jeremiah Mahler]

Loic,

On Sat, Dec 13, 2014 at 05:29:26PM +0100, Loic Pefferkorn wrote:
> Hello,
> 
> The convention for checking for NULL pointers is !ptr and not ptr == NULL.
> This patch fixes such occurences in goldfish driver, it applies against next-20141212
> 
Whose convention is this?  I can't find any mention in
Documention/CodingStyle. checkpatch.pl doesn't complain about them.
And there are almost three thousand examples in staging which don't
use this convention.

  linux-next$ grep -r "== NULL" drivers/staging/* | wc -l
  2844

> 
> Signed-off-by: Loic Pefferkorn <loic@loicp.eu>
> ---
>  drivers/staging/goldfish/goldfish_audio.c |  8 ++++----
>  drivers/staging/goldfish/goldfish_nand.c  | 10 +++++-----
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/goldfish/goldfish_audio.c b/drivers/staging/goldfish/goldfish_audio.c
> index f200359..7ab034b 100644
> --- a/drivers/staging/goldfish/goldfish_audio.c
> +++ b/drivers/staging/goldfish/goldfish_audio.c
> @@ -273,19 +273,19 @@ static int goldfish_audio_probe(struct platform_device *pdev)
>  	dma_addr_t buf_addr;
>  
>  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> -	if (data == NULL)
> +	if (!data)
>  		return -ENOMEM;
>  	spin_lock_init(&data->lock);
>  	init_waitqueue_head(&data->wait);
>  	platform_set_drvdata(pdev, data);
>  
>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (r == NULL) {
> +	if (!r) {
>  		dev_err(&pdev->dev, "platform_get_resource failed\n");
>  		return -ENODEV;
>  	}
>  	data->reg_base = devm_ioremap(&pdev->dev, r->start, PAGE_SIZE);
> -	if (data->reg_base == NULL)
> +	if (!data->reg_base)
>  		return -ENOMEM;
>  
[...]

-- 
- Jeremiah Mahler

Re: [PATCH] staging: goldfish: Fix minor coding style

[Loic Pefferkorn]

> Whose convention is this?  I can't find any mention in
> Documention/CodingStyle. checkpatch.pl doesn't complain about them.
> And there are almost three thousand examples in staging which don't
> use this convention.
> 
>   linux-next$ grep -r "== NULL" drivers/staging/* | wc -l
>   2844

Hi Jeremiah,

Thanks for your feedback.

I have used checkpatch.pl with the --strict flag:

$ ./scripts/checkpatch.pl --strict -f drivers/staging/goldfish/goldfish_nand.c
CHECK: Comparison to NULL could be written "!cps"
#51: FILE: drivers/staging/goldfish/goldfish_nand.c:51:
+   if (cps == NULL)

CHECK: Comparison to NULL could be written "!name"
#333: FILE: drivers/staging/goldfish/goldfish_nand.c:333:
+   if (name == NULL)

CHECK: Comparison to NULL could be written "!r"
#382: FILE: drivers/staging/goldfish/goldfish_nand.c:382:
+   if (r == NULL)

CHECK: Comparison to NULL could be written "!base"
#386: FILE: drivers/staging/goldfish/goldfish_nand.c:386:
+   if (base == NULL)

CHECK: Comparison to NULL could be written "!nand"
#402: FILE: drivers/staging/goldfish/goldfish_nand.c:402:
+   if (nand == NULL)

total: 0 errors, 0 warnings, 5 checks, 442 lines checked

drivers/staging/goldfish/goldfish_nand.c has style problems, please review.

I have also found another commit having the same purpose: 7f376cd6dc1c9bfd14514c70765e6900a961c4b8

-- 
Cheers,
Loïc

Re: [PATCH] staging: goldfish: Fix minor coding style

[One Thousand Gnomes]

On Sat, 13 Dec 2014 17:29:26 +0100
Loic Pefferkorn <loic@loicp.eu> wrote:

> Hello,
> 
> The convention for checking for NULL pointers is !ptr and not ptr == NULL.
> This patch fixes such occurences in goldfish driver, it applies against next-20141212
> 
> 
> Signed-off-by: Loic Pefferkorn <loic@loicp.eu>

Pointless churn. It makes it less readable if anything, and it removes
the type safety as you are now checking against 0 not (void *)0

NAK

Alan

Re: [PATCH] staging: goldfish: Fix minor coding style

[Jeremiah Mahler]

Loïc,

On Sat, Dec 13, 2014 at 07:22:38PM +0100, Loic Pefferkorn wrote:
> > Whose convention is this?  I can't find any mention in
> > Documention/CodingStyle. checkpatch.pl doesn't complain about them.
> > And there are almost three thousand examples in staging which don't
> > use this convention.
> > 
> >   linux-next$ grep -r "== NULL" drivers/staging/* | wc -l
> >   2844
> 
> Hi Jeremiah,
> 
> Thanks for your feedback.
> 
> I have used checkpatch.pl with the --strict flag:
> 
> $ ./scripts/checkpatch.pl --strict -f drivers/staging/goldfish/goldfish_nand.c
> CHECK: Comparison to NULL could be written "!cps"
> #51: FILE: drivers/staging/goldfish/goldfish_nand.c:51:
> +   if (cps == NULL)
> 
> CHECK: Comparison to NULL could be written "!name"
> #333: FILE: drivers/staging/goldfish/goldfish_nand.c:333:
> +   if (name == NULL)
> 
> CHECK: Comparison to NULL could be written "!r"
> #382: FILE: drivers/staging/goldfish/goldfish_nand.c:382:
> +   if (r == NULL)
> 
> CHECK: Comparison to NULL could be written "!base"
> #386: FILE: drivers/staging/goldfish/goldfish_nand.c:386:
> +   if (base == NULL)
> 
> CHECK: Comparison to NULL could be written "!nand"
> #402: FILE: drivers/staging/goldfish/goldfish_nand.c:402:
> +   if (nand == NULL)
> 
> total: 0 errors, 0 warnings, 5 checks, 442 lines checked
> 
> drivers/staging/goldfish/goldfish_nand.c has style problems, please review.
> 
> I have also found another commit having the same purpose: 7f376cd6dc1c9bfd14514c70765e6900a961c4b8
> 
> -- 
> Cheers,
> Loïc

It looks like you're right.  I must say I am surprised.  I had no idea
checkpatch.pl could be even more pedantic than it already is :-)

-- 
- Jeremiah Mahler

Re: [PATCH] staging: goldfish: Fix minor coding style

[Loic Pefferkorn]

On Sat, Dec 13, 2014 at 07:07:05PM +0000, One Thousand Gnomes wrote:
> 
> Pointless churn. It makes it less readable if anything, and it removes
> the type safety as you are now checking against 0 not (void *)0
> 
> NAK
> 
> Alan

The type safety is an interesting point I was not aware of.

Just in case, do you have something for me on your todo list?

-- 
Cheers,
Loïc

Re: [PATCH] checkpatch giving bogus advice (was staging: goldfish: Fix minor coding style)

[One Thousand Gnomes]

On Sat, 13 Dec 2014 11:46:47 -0800
Jeremiah Mahler <jmmahler@gmail.com> wrote:

> Loïc,
> 
> On Sat, Dec 13, 2014 at 07:22:38PM +0100, Loic Pefferkorn wrote:
> > > Whose convention is this?  I can't find any mention in
> > > Documention/CodingStyle. checkpatch.pl doesn't complain about them.
> > > And there are almost three thousand examples in staging which don't
> > > use this convention.
> > > 
> > >   linux-next$ grep -r "== NULL" drivers/staging/* | wc -l
> > >   2844
> > 
> > Hi Jeremiah,
> > 
> > Thanks for your feedback.
> > 
> > I have used checkpatch.pl with the --strict flag:

checkpatch.pl is a bit dubious at the best of times - you can't automate
taste without an AI ;). With --strict it's a positive hazard.

Those kind of small cleanups really only make sense if you are doing big
changes to the code itself anyway and are doing testing and all the rest.

In this case I'd say checkpatch.pl is actually wrong because in the
general case it's better to compare with NULL in C

If you write

          if (!x)

and accidentally use a non-pointer type you don't get a warning. If you
try and compare a non pointer type to NULL you usually do. So the NULL
comparison avoids accidents.

The historical reason for it being done in C was I think to avoid the

          if (x = NULL) 

bug, but gcc will shout at you for that these days.

Alan

Re: staging: goldfish: Fix minor coding style

[One Thousand Gnomes]

On Sat, 13 Dec 2014 22:20:52 +0100
Loic Pefferkorn <loic@loicp.eu> wrote:

> On Sat, Dec 13, 2014 at 07:07:05PM +0000, One Thousand Gnomes wrote:
> > 
> > Pointless churn. It makes it less readable if anything, and it removes
> > the type safety as you are now checking against 0 not (void *)0
> > 
> > NAK
> > 
> > Alan
> 
> The type safety is an interesting point I was not aware of.
> 
> Just in case, do you have something for me on your todo list?

Depends what you feel confident tackling. An interesting but
challenging one to look at as a newcomer might be

https://bugzilla.kernel.org/show_bug.cgi?id=87951

because it provides you with an example .iso file you can loopback mount
to see the crash, and the same fixes were done for a similar bug so can
be seen in the git log to work from.

commit 410dd3cf4c9b36f27ed4542ee18b1af5e68645a4

It's predictable, it's repeatable and it can be done crashing a virtual
machine not a real one. That usually makes things eaiser to fix.

For something easier perhaps

https://bugzilla.kernel.org/show_bug.cgi?id=75111

which just needs the configuration help fixing

Alan

Re: [PATCH] checkpatch giving bogus advice (was staging: goldfish: Fix minor coding style)

[Dan Carpenter]

I haven't seen any bugs caused by lack of type safety with "!foo"...
I prefer !foo because it is more common in the kernel and I think it's
easier to read but I don't feel strongly about this.

I kind of hate "if (foo != NULL) though, because it's a double negative.
But I really hate when people start adding the "!= 0" on to all their
conditions.

	if (frob() != 0)

Also:

	if (a + b != 0)

People do this all the time instead of "if (a || b)" and I don't know
why...

regards,
dan carpenter

Re: staging: goldfish: Fix minor coding style

[Loic Pefferkorn]

On Mon, Dec 15, 2014 at 11:51:47AM +0000, One Thousand Gnomes wrote:
> 
> Depends what you feel confident tackling. An interesting but
> challenging one to look at as a newcomer might be
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=87951
> 
> because it provides you with an example .iso file you can loopback mount
> to see the crash, and the same fixes were done for a similar bug so can
> be seen in the git log to work from.
> 
> commit 410dd3cf4c9b36f27ed4542ee18b1af5e68645a4
> 
> It's predictable, it's repeatable and it can be done crashing a virtual
> machine not a real one. That usually makes things eaiser to fix.
> 
> For something easier perhaps
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=75111
> 
> which just needs the configuration help fixing
> 
> Alan
> 

Hi Alan,

Your answer is greatly appreciated, I'm going to do my best!

-- 
Cheers,
Loïc

Re: [PATCH] checkpatch giving bogus advice (was staging: goldfish: Fix minor coding style)

[Jeremiah Mahler]

On Mon, Dec 15, 2014 at 11:44:21AM +0000, One Thousand Gnomes wrote:
> On Sat, 13 Dec 2014 11:46:47 -0800
> Jeremiah Mahler <jmmahler@gmail.com> wrote:
> 
> > Loïc,
> > 
> > On Sat, Dec 13, 2014 at 07:22:38PM +0100, Loic Pefferkorn wrote:
> > > > Whose convention is this?  I can't find any mention in
> > > > Documention/CodingStyle. checkpatch.pl doesn't complain about them.
> > > > And there are almost three thousand examples in staging which don't
> > > > use this convention.
> > > > 
> > > >   linux-next$ grep -r "== NULL" drivers/staging/* | wc -l
> > > >   2844
> > > 
> > > Hi Jeremiah,
> > > 
> > > Thanks for your feedback.
> > > 
> > > I have used checkpatch.pl with the --strict flag:
> 
> checkpatch.pl is a bit dubious at the best of times - you can't automate
> taste without an AI ;). With --strict it's a positive hazard.
> 
> Those kind of small cleanups really only make sense if you are doing big
> changes to the code itself anyway and are doing testing and all the rest.
> 
> In this case I'd say checkpatch.pl is actually wrong because in the
> general case it's better to compare with NULL in C
> 
> If you write
> 
>           if (!x)
> 
> and accidentally use a non-pointer type you don't get a warning. If you
> try and compare a non pointer type to NULL you usually do. So the NULL
> comparison avoids accidents.
> 
> The historical reason for it being done in C was I think to avoid the
> 
>           if (x = NULL) 
> 
> bug, but gcc will shout at you for that these days.
> 

Or another way mentioned in K&R that produces a compile error

            if (NULL = x) 

> Alan
> 
> 
> 

-- 
- Jeremiah Mahler

Re: [PATCH] checkpatch giving bogus advice (was staging: goldfish: Fix minor coding style)

[Dan Carpenter]

On Mon, Dec 15, 2014 at 05:03:46AM -0800, Jeremiah Mahler wrote:
> 
> Or another way mentioned in K&R that produces a compile error
> 
>             if (NULL = x) 
> 

Yes.  People used to write Yoda code back in the day.  Don't ever do
this in the kernel.

1) It looks stupid.
2) GCC will catch most == vs = bugs as Alan pointed out.
3) There are still some that sneak through because people put double
   parenthesis around everything like "if ((foo = NULL) || (...)) {",
   but checkpatch.pl and Smatch will catch those.

regards,
dan carpenter

Re: [PATCH] checkpatch giving bogus advice (was staging: goldfish: Fix minor coding style)

[Jeremiah Mahler]

Dan,

On Mon, Dec 15, 2014 at 04:43:34PM +0300, Dan Carpenter wrote:
> On Mon, Dec 15, 2014 at 05:03:46AM -0800, Jeremiah Mahler wrote:
> > 
> > Or another way mentioned in K&R that produces a compile error
> > 
> >             if (NULL = x) 
> > 
> 
> Yes.  People used to write Yoda code back in the day.  Don't ever do
> this in the kernel.
> 
> 1) It looks stupid.

Agreed :-)

> 2) GCC will catch most == vs = bugs as Alan pointed out.
> 3) There are still some that sneak through because people put double
>    parenthesis around everything like "if ((foo = NULL) || (...)) {",
>    but checkpatch.pl and Smatch will catch those.
> 
> regards,
> dan carpenter
> 

-- 
- Jeremiah Mahler

Re: [PATCH] checkpatch giving bogus advice (was staging: goldfish: Fix minor coding style)

[Joe Perches]

On Mon, 2014-12-15 at 14:59 +0300, Dan Carpenter wrote:
> I prefer !foo because it is more common in the kernel and I think it's
> easier to read but I don't feel strongly about this.

Me too.  But I do prefer consistency.

fyi: for variants of:

	"if (!foo)"
vs
	"if (foo == NULL)"

a little cocci script shows a preference for "if (!foo)"
of >5:1 in drivers/net/
(actual: 11557:2145)
and a little less (>3:1) in net/
(actual: 10263:3045)

$ cat pointer_style.cocci
@@
type T;
T *a;
@@

*	a == NULL

@@
type T;
T *a;
@@

*	a != NULL

@@
type T;
T *a;
@@

*	!a