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 protected]>
---
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;
Jesper Juhl <[email protected]> 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.
On Tue, 10 May 2005, Andrew Morton wrote:
> Jesper Juhl <[email protected]> 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
"David S. Miller" <[email protected]> wrote:
>
> From: Andrew Morton <[email protected]>
> Subject: Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)
> Date: Tue, 10 May 2005 16:16:57 -0700
>
> > Jesper Juhl <[email protected]> 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.
On Tue, 10 May 2005, Andrew Morton wrote:
> "David S. Miller" <[email protected]> wrote:
> >
> > From: Andrew Morton <[email protected]>
> > Subject: Re: [PATCH] kernel/module.c has something to hide. (whitespace cleanup)
> > Date: Tue, 10 May 2005 16:16:57 -0700
> >
> > > Jesper Juhl <[email protected]> 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
On Tue, 10 May 2005, David S.Miller wrote:
> From: Andrew Morton <[email protected]>
> 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
Jesper Juhl <[email protected]> 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.
Jesper Juhl <[email protected]> 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.
On Tue, 10 May 2005, Andrew Morton wrote:
> Jesper Juhl <[email protected]> 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
On Tue, 2005-05-10 at 17:29 -0700, Andrew Morton wrote:
> Jesper Juhl <[email protected]> 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
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
Jesper Juhl wrote:
> On Tue, 10 May 2005, Andrew Morton wrote:
>
>>Jesper Juhl <[email protected]> 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 - http://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)
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 <[email protected]> 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 - http://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
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 - http://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)
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 - http://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)
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
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