login
Header Space

 
 

Re: [PATCH 109/148] include/asm-x86/serial.h: checkpatch cleanups - formatting only

Score:
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
To: Ingo Molnar <mingo@...>
Cc: David Miller <davem@...>, <jirislaby@...>, <viro@...>, <joe@...>, <tglx@...>, <linux-kernel@...>
Date: Tuesday, March 25, 2008 - 8:26 am

Ingo Molnar <mingo@elte.hu> writes:

You assert that all the time, but it is just that: an assertation.
I assert that code style is only a small part of code correctness.
Also just an assertation. Who is more right? Probably the truth 
is somewhere inbetween. At least I think it is nearer my position
than yours @)

Also regarding the rules enforced by checkpatch I think there is a wide
range on how much they impact readability: e.g. if someone uses
the wrong bracket style consistently that is somewhat disrupting.
I agree.

But is trailing white space disrupting to code reading in any 
way? Very doubtful. 

Most rules are somewhere inbetween. They vary widely in how
much they impact readability.

Also sometimes the rules conflict. Example: the 80 column rule
often conflicts with the "always space around operator" rule.
That is because expressions split over multiple lines are harder
to read than an expression on a single line (at least here) and at 
least I would rather trade a few missing spaces around operators
than to have a multi-line expression.

It's always a trade-off and checkpatch.pl is not very good
(read it doesn't really handle) trade-offs.


For new code being added (like your CFS scheduler) it is fine, but for
old code it has the problem of conflicting with other actually useful
changes on the same areas which are pending.  And doing merges into
such changing code bases is always somewhat error prone because the people
who do it are also only human and can apply subtle typos etc.  

Strictly seen each such merge requires a whole new testing cycle and
doing such a testing cycle just for someone's checkpatch changes is
really a waste of time and seriously impacting real progress. 
The only saving grace is that it will hopefully only happen once
per file, but the point still holds. There are a lot of different files
in Linux, so it has the potential to be a serious problem.

That is an objective (not just random assertation) reason against
doing extensive changes of existing files like Joe's patchkit.

I think it would be fair at least if people doing this asked first at least:
- Does anybody have pending changes against file X, perhaps
also checking mm and linux-next 
and then wait a bit and if someone says he has pending changes then not do 
the reformatting until the pending changes get merged.

Or better really only do it on new code.

-Andi
--
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
Re: [PATCH 109/148] include/asm-x86/serial.h: checkpatch cle..., Andi Kleen, (Tue Mar 25, 8:26 am)
Re: [PATCH 109/148] include/asm-x86/serial.h: checkpatch cle..., Christoph Hellwig, (Wed Mar 26, 7:09 am)
[patch] bkl2mtd: cleanup, Ingo Molnar, (Wed Mar 26, 6:14 am)
Re: [patch] bkl2mtd: cleanup, Al Viro, (Wed Mar 26, 6:48 am)
Re: [patch] bkl2mtd: cleanup, Ingo Molnar, (Wed Mar 26, 7:10 am)
Re: [patch] bkl2mtd: cleanup, Joe Perches, (Wed Mar 26, 12:30 pm)
Re: [patch] bkl2mtd: cleanup, Jiri Slaby, (Wed Mar 26, 7:14 am)
Re: [patch] bkl2mtd: cleanup, Ingo Molnar, (Wed Mar 26, 7:02 am)
Re: [patch] bkl2mtd: cleanup, Ingo Molnar, (Wed Mar 26, 7:00 am)
Re: [patch] bkl2mtd: cleanup, Jörn, (Wed Mar 26, 6:57 am)
speck-geostationary