[PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Jesper Juhl]

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 <juhl-lkml@dif.dk>
---

 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;

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Andrew Morton]

Jesper Juhl <juhl-lkml@dif.dk> 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.

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Jesper Juhl]

On Tue, 10 May 2005, Andrew Morton wrote:

> Jesper Juhl <juhl-lkml@dif.dk> 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

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Andrew Morton]

"David S. Miller" <davem@davemloft.net> wrote:
>
> From: Andrew Morton <akpm@osdl.org>
> Subject: Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)
> Date: Tue, 10 May 2005 16:16:57 -0700
> 
> > Jesper Juhl <juhl-lkml@dif.dk> 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.

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Jesper Juhl]

On Tue, 10 May 2005, Andrew Morton wrote:

> "David S. Miller" <davem@davemloft.net> wrote:
> >
> > From: Andrew Morton <akpm@osdl.org>
> > Subject: Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)
> > Date: Tue, 10 May 2005 16:16:57 -0700
> > 
> > > Jesper Juhl <juhl-lkml@dif.dk> 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

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Andrew Morton]

Jesper Juhl <juhl-lkml@dif.dk> 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.

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Jesper Juhl]

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

> From: Andrew Morton <akpm@osdl.org>
> 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

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Andrew Morton]

Jesper Juhl <juhl-lkml@dif.dk> 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.

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Jesper Juhl]

On Tue, 10 May 2005, Andrew Morton wrote:

> Jesper Juhl <juhl-lkml@dif.dk> 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.
> 
Ok, I'll try and split the tree into some sane chunks and you'll get the 
patches in a steady stream.. Expect the first few in a few days.

> >  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.
> 
Ok, will do.

-- 
Jesper

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Paulo Marques]

Jesper Juhl wrote:
> On Tue, 10 May 2005, Andrew Morton wrote:
> 
>>Jesper Juhl <juhl-lkml@dif.dk> wrote:
>>
>>>[...]
>>> 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.
>>
>>[...]
> 
> Ok, will do.

Just a small sugestion: do a sha (or md5sum, or whatever hash function 
you prefer) to vmlinux before and after applying the patches.

If all is well, it shouldn't change (since this is just whitespace 
cleanup), and it is a little more robust than just checking the size.

-- 
Paulo Marques - www.grupopie.com

An expert is a person who has made all the mistakes that can be
made in a very narrow field.
Niels Bohr (1885 - 1962)

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Adrian Bunk]

On Wed, May 11, 2005 at 12:07:55PM +0100, Paulo Marques wrote:
> Jesper Juhl wrote:
> >On Tue, 10 May 2005, Andrew Morton wrote:
> >
> >>Jesper Juhl <juhl-lkml@dif.dk> wrote:
> >>
> >>>[...]
> >>>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.
> >>
> >>[...]
> >
> >Ok, will do.
> 
> Just a small sugestion: do a sha (or md5sum, or whatever hash function 
> you prefer) to vmlinux before and after applying the patches.
> 
> If all is well, it shouldn't change (since this is just whitespace 
> cleanup), and it is a little more robust than just checking the size.


That's wrong.

vmlinux contains the date of the compilation.


> Paulo Marques - www.grupopie.com


cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Paulo Marques]

Adrian Bunk wrote:
> On Wed, May 11, 2005 at 12:07:55PM +0100, Paulo Marques wrote:
> [...]
>>Just a small sugestion: do a sha (or md5sum, or whatever hash function 
>>you prefer) to vmlinux before and after applying the patches.
>>
>>If all is well, it shouldn't change (since this is just whitespace 
>>cleanup), and it is a little more robust than just checking the size.
> 
> That's wrong.
> 
> vmlinux contains the date of the compilation.

You're right, I forgot about that...

Removing UTS_VERSION from init/version.c would make this work, or are 
there other places where this might be a problem?

-- 
Paulo Marques - www.grupopie.com

An expert is a person who has made all the mistakes that can be
made in a very narrow field.
Niels Bohr (1885 - 1962)

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Paulo Marques]

Paulo Marques wrote:
> Adrian Bunk wrote:
> 
>> On Wed, May 11, 2005 at 12:07:55PM +0100, Paulo Marques wrote:
>> [...]
>>
>>> Just a small sugestion: do a sha (or md5sum, or whatever hash 
>>> function you prefer) to vmlinux before and after applying the patches.
>>>
>>> If all is well, it shouldn't change (since this is just whitespace 
>>> cleanup), and it is a little more robust than just checking the size.
>>
>> That's wrong.
>>
>> vmlinux contains the date of the compilation.
> 
> You're right, I forgot about that...
> 
> Removing UTS_VERSION from init/version.c would make this work, or are 
> there other places where this might be a problem?

Ok, I've just tested this.

At least with my config, if I remove both instances of UTS_VERSION from 
init/version.c, the resulting vmlinux files are exactly identical with 
the same sha1sum.

So maybe Jesper can use this to make *really* sure that there are no 
actual changes with the patches, just whitespace changes.

-- 
Paulo Marques - www.grupopie.com

An expert is a person who has made all the mistakes that can be
made in a very narrow field.
Niels Bohr (1885 - 1962)

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Jesper Juhl]

On Thu, 12 May 2005, Paulo Marques wrote:

> Paulo Marques wrote:
> > Adrian Bunk wrote:
> > 
> > > On Wed, May 11, 2005 at 12:07:55PM +0100, Paulo Marques wrote:
> > > [...]
> > > 
> > > > Just a small sugestion: do a sha (or md5sum, or whatever hash function
> > > > you prefer) to vmlinux before and after applying the patches.
> > > > 
> > > > If all is well, it shouldn't change (since this is just whitespace
> > > > cleanup), and it is a little more robust than just checking the size.
> > > 
> > > That's wrong.
> > > 
> > > vmlinux contains the date of the compilation.
> > 
> > You're right, I forgot about that...
> > 
> > Removing UTS_VERSION from init/version.c would make this work, or are there
> > other places where this might be a problem?
> 
> Ok, I've just tested this.
> 
> At least with my config, if I remove both instances of UTS_VERSION from
> init/version.c, the resulting vmlinux files are exactly identical with the
> same sha1sum.
> 
> So maybe Jesper can use this to make *really* sure that there are no actual
> changes with the patches, just whitespace changes.
> 

Yeah, that does seem to work. I had not thought of that - thanks.


/Jesper Juhl

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Hugh Dickins]

On Thu, 12 May 2005, Jesper Juhl wrote:
> On Thu, 12 May 2005, Paulo Marques wrote:
> > 
> > At least with my config, if I remove both instances of UTS_VERSION from
> > init/version.c, the resulting vmlinux files are exactly identical with the
> > same sha1sum.
> > 
> > So maybe Jesper can use this to make *really* sure that there are no actual
> > changes with the patches, just whitespace changes.
> 
> Yeah, that does seem to work. I had not thought of that - thanks.

If you're going so far as to change the line numbering (which is very
much your intention!), then you'll also need to suppress any __LINE__s
for that check.  Switching off CONFIG_DEBUG_BUGVERBOSE should suppress
most of them.

Hugh

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Tom Duffy]

[-- Attachment #1: Type: text/plain, Size: 1059 bytes --]

On Tue, 2005-05-10 at 17:29 -0700, Andrew Morton wrote:
> Jesper Juhl <juhl-lkml@dif.dk> 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?

Solaris build makes sure files passes a "lint" test during the build and
nothing can be checked in until such a test can pass.

Would it make sense to add such a test during kernel compile for Linux?
Something that could be turned off if somebody needed really fast
builds.  This would check for things like whitespace violations and
other things that violate CodingStyle.

People tend to fix things quick if they break the build.

-tduffy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Adrian Bunk]

On Tue, May 10, 2005 at 06:01:03PM -0700, Tom Duffy wrote:
> 
> Solaris build makes sure files passes a "lint" test during the build and
> nothing can be checked in until such a test can pass.
> 
> Would it make sense to add such a test during kernel compile for Linux?
> Something that could be turned off if somebody needed really fast
> builds.  This would check for things like whitespace violations and
> other things that violate CodingStyle.
> 
> People tend to fix things quick if they break the build.

This works _after_ the kernel has been cleaned up.

And then there's the issue that some code (e.g. ACPI or XFS) is shared 
between Linux and other OS's, and therefore a limited amount of 
divergence from usual kernel coding style is allowed in such code.

> -tduffy

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Bodo Eggert <harvested.in.lkml@posting.7eggert.dyndns.org>]

Jesper Juhl <juhl-lkml@dif.dk> wrote:

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

> - (to a limited degree) no trailing whitespace

I just ran a script over -rc4 to remove trailing ws. The result is
about 22 MB in 429 patches (iterated over ./*/*).

How hard can I patch you before you start patching me?

Which addresses am I supposed to send it to? I don't want to break the
record for the most annoying patch series in lkml.
-- 
If at first you don't succeed, call it version 1.0 

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Nick Piggin]

Bodo Eggert <harvested.in.lkml@posting.7eggert.dyndns.org> wrote:

>Jesper Juhl <juhl-lkml@dif.dk> wrote:
>
>
>>If Andrew agrees, then I'll commit to doing this cleanup;
>>
>
>>- (to a limited degree) no trailing whitespace
>>
>
>I just ran a script over -rc4 to remove trailing ws. The result is
>about 22 MB in 429 patches (iterated over ./*/*).
>
>How hard can I patch you before you start patching me?
>
>Which addresses am I supposed to send it to? I don't want to break the
>record for the most annoying patch series in lkml.
>

First of all, why is it 429 patches? The patches we want aren't about a
file or a subdirectory or even a subsystem, but they're supposed to be
a logical change. Ie. 1 patch. An exception for something like this would
be if you want to feed it to different maintainers seperately, but it
sounds like you just want to bomb it somewhere...

Secondly, let's not.

More subtle indenting stuff like fixing `if (exp) do_something` I can
understand. But this isn't nearly so helpful.

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Bodo Eggert]

On Thu, 12 May 2005, Nick Piggin wrote:
> Bodo Eggert <harvested.in.lkml@posting.7eggert.dyndns.org> wrote:
> >Jesper Juhl <juhl-lkml@dif.dk> wrote:

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

> >>- (to a limited degree) no trailing whitespace

> >I just ran a script over -rc4 to remove trailing ws. The result is
> >about 22 MB in 429 patches (iterated over ./*/*).
> >
> >How hard can I patch you before you start patching me?
> >
> >Which addresses am I supposed to send it to? I don't want to break the
> >record for the most annoying patch series in lkml.
> >
> 
> First of all, why is it 429 patches?

Because
1) some parts will get rejected due to conflicting patches. Only those 
parts will need to be recreated.
2) i forgot to create the 430th patch.

> The patches we want aren't about a
> file or a subdirectory or even a subsystem, but they're supposed to be
> a logical change. Ie. 1 patch.

That would be too large for most mailboxes. If you like a single patch,
you can just concatenate all the patches, so splitting it was a safe bet.

> An exception for something like this would
> be if you want to feed it to different maintainers seperately, but it
> sounds like you just want to bomb it somewhere...

I asume there is no automatic way to get the maintainer from a given 
file, and I don't want to grow old and grey while doing that manually.

-- 
"Bravery is being the only one who knows you're afraid."
-David Hackworth

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Jesper Juhl]

On Thu, 12 May 2005, Bodo Eggert <harvested.in.lkml@posting.7eggert.dyndns.org> wrote:

> Date: Thu, 12 May 2005 01:47:28 +0200
> From: "Bodo Eggert <harvested.in.lkml@posting.7eggert.dyndns.org>"
>     <7eggert@gmx.de>
> To: Jesper Juhl <juhl-lkml@dif.dk>, David S.Miller <davem@davemloft.net>,
>     Andrew Morton <akpm@osdl.org>, linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] kernel/module.c has something to hide. (whitespace 
>     cleanup)
> 
> Jesper Juhl <juhl-lkml@dif.dk> wrote:
> 
> > If Andrew agrees, then I'll commit to doing this cleanup;
> 
> > - (to a limited degree) no trailing whitespace
> 
> I just ran a script over -rc4 to remove trailing ws. The result is
> about 22 MB in 429 patches (iterated over ./*/*).
> 
Ok, I'm not looking at trailing ws yet. I'm working on the "two statements 
on one line" and "spaces between function name and opening paren" bits. 
It's a bit slow going since I don't have many hours a day untill the 
weekend, but I hope to have the first batch of patches ready over the 
weekend.

> How hard can I patch you before you start patching me?
> 
Let me have a copy of your patches, then I can do my work incremental to 
yours.  If you could tar & gz them and attach them to a mail and send it 
to me in private email that would be perfect.


/Jesper

Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)

[Andrew Morton]

Jesper Juhl <juhl-lkml@dif.dk> wrote:
>
>  Ok, I'm not looking at trailing ws yet.

Obviously the way to handle this is to write a script and send it to Linus.

Good luck ;)