Jesper Juhl submitted a small patch to bring the kernel/module.c source file closer in line with the kernel's CodingStyle 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] 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.
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 [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 [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 [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 [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 [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 [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 [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.
| Attachment | Size |
|---|---|
| CodingStyle.txt | 14.99 KB |
What happened to Lindent?
A script should fix most of this stuff, and that's enough!
obsolete it...
Why not just enforce the style for all new patches and let nature take its course?
indent
there's a pretty program called indent.
Just run it.
I found that indent and tidy,
I found that indent and tidy, a C and a HTML code "beautifier", respectively, increase my productiveness by about 30% becuase I can write in any messy style I want and don't have to care about indentation in patches, just run them once in a while. That's probably as much gain in productiveness as is achieved by using, say, BitKeeper instead of CVS, seriously. So use them!
Not a big fan of indent.
I'm not a big fan of indent. It's good for rescuing tab-damaged code from its formatting entropy, but after that I find doing it right the first time for any subsequent code to be much more efficient than relying on a tool.
Part of the reason is that I write things very structurally, lining up common elements on consecutive lines, and so on. My programs almost look tabular in places--like a spreadsheet. It makes it really easy to pick out what's going on. Programs like indent destroy all that.
I don't get it
Statements are separated by semicolons in C.
if (a == b) return(1);
is exactly one statement and the rule "one statement per line" should mean the above line is the preferred syntax.
Still, the patch changes this to two lines. Why?
That's really easy to answer.
That's really easy to answer. The problem is not
if(a==b) return(1);
The problem is
if(a==b) return(1);
do_something_else();
Which to the naked eye might parse to:
if(a==b) //garbage
do_something_else();
In short, breaking this up improves readability for a lot of people, especially since right now both styles are used.
But the second example has it
But the second example has its indentation all wrong, you should not be able to read it that way?
Anyway, so what CodingStyle is really saying is that "one statement per line, except the ones which some people split in two which we'll split in two lines to make sure they read it right".
>Statements are separated by
>Statements are separated by semicolons in C.
>if (a == b) return(1);
>is exactly one statement and the rule "one statement per line" should >mean the above line is the preferred syntax.
Heh... It's just one side of the truth. According to you the following should be OK:
for(int i = 0; i < 10; i++) { do_something1(); do_something_2();}
It's a statement too :) The truth is that we are speaking about basic statements: one statement means one action or something like that.
Yeah, the for loop statement
Yeah, the for loop statement would be an exception. But I don't consider a block to be just one statement (thus main() isn't as well).
Wrong Interpretation
Statements are separated by semicolons in C.
if (a == b) return(1);
is exactly one statement and the rule "one statement per line" should mean the above line is the preferred syntax.
Still, the patch changes this to two lines. Why?
---
The rule "one statement per line" is not meant to compact code; it is meant to expand it. More specifically, its aimed at to keep code like "tmp = x; x = y; y = tmp;" from being on the same line.
Another section of the coding style document points out that lines should not be more than 80 characters long. This affects long strings, and they must be broken into multiple lines. Multiple lines - still one statement.
Also, keep whitespace in mind.... even blank lines are still lines and would violate the rules according to that interpretation.
I see! It's "not more than on
I see! It's "not more than one line per statement", not "exactly one line per statement".
It's more like "not more than
It's more like "not more than one statement per line".
Eesh, I shudder to think of t
Eesh, I shudder to think of the number of bugs they'll introduce by doing this en masse. I remember when I was porting Soldier of Fortune to Linux and I was determined to "correct" warnings in the build, and "fixed" a cast issue that caused transmission of float values over the wire to break.
m.
There is a difference. They a
There is a difference. They are not fixing any bugs. All of these patches are only changing the whitespace. Nothing more.
Yes, it should be trivial to
Yes, it should be trivial to test the patches -- just compile before and after and diff the binaries.
I've introduced bugs by chang
I've introduced bugs by changing whitespace, because I unintentionally changed some of the non-whitespace as well.
If we had a tool that verified two files were equivalent save for whitespace, that'd be awesome. Diff doesn't quite do that, because of its line orientation. I hear Emacs has a diff mode that understands whitespace across line boundaries. That might be useful here.
diff the .o
Just run diff or cmp on the .o files. They should stay the same before and after the fix.
Doesn't quite work unless the
Doesn't quite work unless they're stripped of debug information, since line numbers get encoded in that. Also, on some platforms, doesn't the assembler embed a timestamp?
Only if you build without opt
Only if you build without optimization, and maybe not even then. GCC is not deterministic, and I know from experience that some of the optimization techniques in the compiler produce slightly different code on every run.
That shouldn't happen. I d
That shouldn't happen.
I do know, though, that very minor tweaks to code, such as reordering two apparently independent expressions, can lead to very noticeable shifts in the optimized output. That has more to do with changing the preconditions that many of their heuristics see.
Also, depending on your system, any static libraries you build might be different build-to-build due to the ranlib tool. Same may be true of the final linked object. But the individual .o's? No.
FWIW, I just built one of my projects twice in a row with GCC 4.0.0, and the two binaries are bit-exact. In fact, changing white space in the source doesn't seem to be a problem either until I turn on -g (since the line-number information switches around).
If your gcc is not determinis
If your gcc is not deterministic, then you have a problem. Most likely you've hit a bug in gcc or your memory is faulty. It might be a good idea to examine it a bit more.
No magic needed
To compare both source codes just pass both of them to indent with the most bizarre options (maybe the default? ;-) and diff the output. You can also use a sed command that just strips *all* white space.
It's trivial to make sure the
It's trivial to make sure these patches don't affect the actual code; just recompile it and make sure the built kernel binary matches the one you got before.
-Zorin
re: It's trivial to make sure the
Actually, what if source code line numbers are inserted into the object and binary files by the compiler, something that wouldn't be uncommon for a large and technical software project like the linux kernel.
They aren't, so it's not a pr
They aren't, so it's not a problem.