logo
Published on KernelTrap (http://kerneltrap.org)

Linux: Cleaning Up Per The Kernel Coding Style

By Jeremy
Created May 11 2005 - 19:02

Jesper Juhl submitted a small patch [1] to bring the kernel/module.c source file closer in line with the kernel's CodingStyle [2] document. Specifically, he quoted from the CodingStyle document, "don't put multiple statements on a single line unless you have something to hide," which goes on to give an example of how such statements can cause confusion:

         if (condition) do_this;
           do_something_everytime;

2.6 maintainer Andrew Morton [interview [3]] quickly replied, "there are about 88 squillion of these in the kernel. I think it would be a mistake for me to start taking such patches, sorry." David Miller countered, "I disagree. Putting statements on the same line as the if statement hides bugs and makes the code harder to read." Andrew replied, "we all know that, but this means that we spend the next two years fielding an ongoing dribble of trivial patches which distract from real work."

A short discussion followed in which Andrew agreed, "well I suppose I could live with a few REALLY REALLY BIG patches to do this to lots of files, but if it's the old death-by-1000-cuts, I'm gonna call uncle this time." Jesper agreed to begin working on the large patches, to which Andrew repeated, "OK, a few 100k-400k patches would suit." A similar discussion was raised here [4].

From: Jesper Juhl [email blocked]
To:  linux-kernel
Subject: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)
Date:	Wed, 11 May 2005 01:02:16 +0200 (CEST)


Quoting Documentation/CodingStyle : 
        "Don't put multiple statements on a single line unless you have
         something to hide:
         
         if (condition) do_this;
           do_something_everytime;
        "

Signed-off-by: Jesper Juhl [email blocked]
---

 kernel/module.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

--- linux-2.6.12-rc3-mm3-orig/kernel/module.c	2005-05-06 23:21:28.000000000 +0200
+++ linux-2.6.12-rc3-mm3/kernel/module.c	2005-05-11 00:56:54.000000000 +0200
@@ -410,7 +410,8 @@ static int already_uses(struct module *a
 static int use_module(struct module *a, struct module *b)
 {
 	struct module_use *use;
-	if (b == NULL || already_uses(a, b)) return 1;
+	if (b == NULL || already_uses(a, b))
+		return 1;
 
 	if (!strong_try_module_get(b))
 		return 0;
@@ -1731,9 +1732,10 @@ static struct module *load_module(void _
 	kfree(args);
  free_hdr:
 	vfree(hdr);
-	if (err < 0) return ERR_PTR(err);
-	else return ptr;
-
+	if (err < 0)
+		return ERR_PTR(err);
+	else
+		return ptr;
  truncated:
 	printk(KERN_ERR "Module len %lu truncated\n", len);
 	err = -ENOEXEC;



From: Andrew Morton [5] [email blocked]
Subject: Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)
Date:	Tue, 10 May 2005 16:16:57 -0700

Jesper Juhl [email blocked] wrote:
>
> -	if (b == NULL || already_uses(a, b)) return 1;
> +	if (b == NULL || already_uses(a, b))
> +		return 1;

There are about 88 squillion of these in the kernel.  I think it would be a
mistake for me to start taking such patches, sorry.



From: Jesper Juhl [email blocked]
Subject: Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)
Date:	Wed, 11 May 2005 01:30:39 +0200 (CEST)

On Tue, 10 May 2005, Andrew Morton wrote:

> Jesper Juhl [email blocked] wrote:
> >
> > -	if (b == NULL || already_uses(a, b)) return 1;
> > +	if (b == NULL || already_uses(a, b))
> > +		return 1;
> 
> There are about 88 squillion of these in the kernel.  I think it would be a
> mistake for me to start taking such patches, sorry.
> 
Yes, there are a ton of these. There are also a ton of files that use 
spaces instead of tabs, and a ton of files that use spaces between 
function name and opening parenthesis - like  foo (arg);  or even 
 foo ( arg ) ;

Just because there are lost of them doesn't (in my oppinion) mean that 
they shouldn't be cleaned up. Reading code that doesn't adhere to 
CodingStyle is annoying at best, and hides bugs at worst.
There's no way I'm going to clean it all up - especially not if you don't 
want the patches, but I think it *ought* to be cleaned up, and if you 
should change your mind then I'm willing to clean up at least a fair bit 
of it.


-- 
Jesper



From: Andrew Morton [6] [email blocked]
Subject: Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)
Date:	Tue, 10 May 2005 17:02:46 -0700

"David S. Miller" [email blocked] wrote:
>
> From: Andrew Morton [7] [email blocked]
> Subject: Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)
> Date: Tue, 10 May 2005 16:16:57 -0700
> 
> > Jesper Juhl [email blocked] wrote:
> > >
> > > -	if (b == NULL || already_uses(a, b)) return 1;
> > > +	if (b == NULL || already_uses(a, b))
> > > +		return 1;
> > 
> > There are about 88 squillion of these in the kernel.  I think it would be a
> > mistake for me to start taking such patches, sorry.
> 
> I disagree.  Putting statements on the same line as
> the if statement hides bugs and makes the code harder
> to read.

We all know that, but this means that we spend the next two years fielding
an ongoing dribble of trivial patches which distract from real work.

> Fixing these makes the kernel eaiser to maintain
> and debug.

Well I suppose I could live with a few REALLY REALLY BIG patches to do this
to lots of files, but if it's the old death-by-1000-cuts, I'm gonna call
uncle this time.



From: Jesper Juhl [email blocked]
Subject: Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)
Date:	Wed, 11 May 2005 02:11:48 +0200 (CEST)

On Tue, 10 May 2005, Andrew Morton wrote:

> "David S. Miller" [email blocked] wrote:
> >
> > From: Andrew Morton [8] [email blocked]
> > Subject: Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)
> > Date: Tue, 10 May 2005 16:16:57 -0700
> > 
> > > Jesper Juhl [email blocked] wrote:
> > > >
> > > > -	if (b == NULL || already_uses(a, b)) return 1;
> > > > +	if (b == NULL || already_uses(a, b))
> > > > +		return 1;
> > > 
> > > There are about 88 squillion of these in the kernel.  I think it would be a
> > > mistake for me to start taking such patches, sorry.
> > 
> > I disagree.  Putting statements on the same line as
> > the if statement hides bugs and makes the code harder
> > to read.
> 
> We all know that, but this means that we spend the next two years fielding
> an ongoing dribble of trivial patches which distract from real work.
> 
That was not the plan. The patch at the start of this thread was merely a 
"feeler" to see what the judge the reaction to such patches.

> > Fixing these makes the kernel eaiser to maintain
> > and debug.
> 
> Well I suppose I could live with a few REALLY REALLY BIG patches to do this
> to lots of files, but if it's the old death-by-1000-cuts, I'm gonna call
> uncle this time.
> 
These things annoy me, so if you are willing to accept a few patches (in 
the 10-20 range) that clean most (I'm not going to say all) of this stuff 
up, then I'm game. Give me a few days (more likely a week or two or 
slightly more) and I'll get that done. Those patches *will* be big 
though...

--
Jesper



From: Andrew Morton [9] [email blocked]
Subject: Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)
Date:	Tue, 10 May 2005 17:23:32 -0700

Jesper Juhl [email blocked] wrote:
>
> > 
> > Well I suppose I could live with a few REALLY REALLY BIG patches to do this
> > to lots of files, but if it's the old death-by-1000-cuts, I'm gonna call
> > uncle this time.
> > 
> These things annoy me, so if you are willing to accept a few patches (in 
> the 10-20 range) that clean most (I'm not going to say all) of this stuff 
> up, then I'm game. Give me a few days (more likely a week or two or 
> slightly more) and I'll get that done. Those patches *will* be big 
> though...

OK, a few 100k-400k patches would suit.

Make the patches against latest -linus tree.  I'll then apply them on top
of latest -mm, discard all the rejects and then rediff against -linus.

That way we end up with a patch which minimises the number of conflicts
with other people's ongoing work.



From: Jesper Juhl [email blocked]
Subject: Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)
Date:	Wed, 11 May 2005 02:28:05 +0200 (CEST)

On Tue, 10 May 2005, David S.Miller wrote:

> From: Andrew Morton [10] [email blocked]
> Subject: Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)
> Date: Tue, 10 May 2005 17:02:46 -0700
> 
> > Well I suppose I could live with a few REALLY REALLY BIG patches to do this
> > to lots of files, but if it's the old death-by-1000-cuts, I'm gonna call
> > uncle this time.
> 
> Fair enough.  One should be able to regex the majority of them
> with a check for "if *(" then ";" on the same line.
> 
Indeed.

If Andrew agrees, then I'll commit to doing this cleanup;

- no two statements on same line
- no funky spaces between function names, arguments etc.
- no spaces instead of proper tabs.
- (to a limited degree) no trailing whitespace
- perhaps other whitespace cleanups as per Codingstyle

For the entire tree.

No actual code changes - should be resonably easy to review.

Some can be semi-automated, some can't, and it'll take time and be a 
royal pain. But if Andrew will take the patches I'll do it (in some 2 
digit nr of patches (aiming for <50 - obviously guessing wildly here)).

I'll need time to do this - no matter how you cut it there are a lot of 
files, and a lot of lines - so don't expect the patch bombing to start for 
the next few weeks.
And before I embark on this venture I'd like some feedback that when I do 
turn up with patches they'll have a resonable chance of getting merged - 
this is going to be a lot of boring work, and with no commitment to merge 
anything it's not something I want to waste days on...  Sounds fair?

Ohh, and I'd be submitting all the patches to you Andrew, not individual 
maintainers/authors..


-- 
Jesper



From: Andrew Morton [11] [email blocked]
Subject: Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)
Date:	Tue, 10 May 2005 17:29:13 -0700

Jesper Juhl [email blocked] wrote:
>
> I'll need time to do this - no matter how you cut it there are a lot of 
>  files, and a lot of lines - so don't expect the patch bombing to start for 
>  the next few weeks.
>  And before I embark on this venture I'd like some feedback that when I do 
>  turn up with patches they'll have a resonable chance of getting merged - 
>  this is going to be a lot of boring work, and with no commitment to merge 
>  anything it's not something I want to waste days on...  Sounds fair?

ho hum.  Just send them over as you generate them.

>  Ohh, and I'd be submitting all the patches to you Andrew, not individual 
>  maintainers/authors..

That should be OK - you can test that the .o files have the same `size'
output before-and-after.

The changes shouldn't break any subsystem maintainers' trees, although they
will surely trip up individual developers who are working on stuff, so
please make some attempt to keep the relevant people in the loop.


Related Links:

Source URL:
http://kerneltrap.org/node/5100

Links:
[1] http://kerneltrap.org/mailarchive/1/message/64343/thread
[2] http://kerneltrap.org/files/Jeremy/CodingStyle.txt
[3] http://kerneltrap.org/node/view/10
[4] http://www.joelonsoftware.com/articles/Wrong.html
[5] http://kerneltrap.org/node/view/10
[6] http://kerneltrap.org/node/view/10
[7] http://kerneltrap.org/node/view/10
[8] http://kerneltrap.org/node/view/10
[9] http://kerneltrap.org/node/view/10
[10] http://kerneltrap.org/node/view/10
[11] http://kerneltrap.org/node/view/10
[12] http://kerneltrap.org/node/view/10