From: torvalds@transmeta.com (Linus Torvalds)
Subject: Re: Coding Style
Date: 	19 Jan 2001 21:34:42 -0800
Newsgroups: fa.linux.kernel

In article <200101200046.f0K0kgj201065@saturn.cs.uml.edu>,
Albert D. Cahalan <acahalan@cs.uml.edu> wrote:
>> Tabs are 8 characters so NO tabs should be used in ANY source file what 
>...
>> Rationale:  Tabs force your code out to the right edge of the display
>> leaving no room for comments.  You don't need great big gaping spaces to
>> delineate the start and end of a block, TWO spaces do this just fine.
>
>Correct, because adjustable tab width is a myth. The comments don't
>line up when you muck with tab width.

Read the Linux CodingStyle.

Tabs are 8 characters. They are NOT adjustable. Never have been, never
will be. Anybody who thinks tabs are anything but 8 chars apart is just
wrong. It's that simple.

And two spaces is not enough. If you write code that needs comments at
the end of a line, your code is crap. It's that easy. There is _never_ a
reason to comment a single line, and multi-line comments the the right
of multi-line code to the left is a recipe for disaster. In short, you
don't do comments to the right of code - you do them _before_ code.

			Linus

Date: 	Fri, 19 Jan 2001 22:29:35 -0800 (PST)
From: Linus Torvalds <torvalds@transmeta.com>
Subject: Re: Coding Style
Newsgroups: fa.linux.kernel

On Fri, 19 Jan 2001, Mark I Manning IV wrote:
> 
> Two spaces are perfect, they delineate the blocks very nicely and dont
> eat up the comments real estate.

WHAT "comments real estate". You have tons of real estate - up and down.
Don't try to move it sideways where it won't fit anyway.

Write comments before your function. If your function is so long and
complicated that you think it needs comments after the lines, then you
should have split it up. See "function growth hormone imbalance".

> > And two spaces is not enough. If you write code that needs comments at
> > the end of a line, your code is crap.
> 
> Might i ask you to qualify that statement ?

Ok. I'll qualify it. Add a "unless you write in assembly language" to the
end. I have to admit that most assembly languages are terse and hard to
read enough that you often want to comment at the end. In assembly you
just don't tend to have enough flexibility to make the code readable, so
you must live with unreadable code that is commented.

> I disagree, comments should be associated with the code they are
> describing, putting a block of comments before a function telling people
> waht it does does nothing to tell them HOW it does it.  

Just add a "how it does it" section to your comment.

Or add your comments inside the function if you really want to localize
them, but do it something like this:

	/*
	 * my_integer_pi()
	 *
	 * This function calculates the value of PI, and returns
	 * 3. It does so by adding "1" in a loop three times.
	 */
	int my_integer_pi(void)
	{
		int i, pi;

		/*
		 * This is the main loop.
		 */
		pi = 0;
		for (i = 0; i < 3; i++)
			pi++;

		/* Ok, return it */
		return pi;
	}

Notice? Not AFTER the statements. 

Why? Because you are likely to want to change the statements. You don't
want to keep moving the comments around to line them up. And trying to
have a multi-line comment with code is just HORRIBLE:

		pi = 0;			/* Initialize our counter  */
		for (i = 0; i < 3; i++)	/* to zero before the loop */
			pi++;		/* Increment it.	   */

because the above turns into absolute mush if you change your code a bit
(maybe you decide to move the initialization up a bit, because you make
your function start off with a better approximation, and you want to use
another algorithm to calculate PI to more than zero decimal points. Now
you need to move the two-line comment around, and break it up etc etc.

In contrast, when you have comments on lines of their own, you just move
the whole comment block when you re-organize the code.

		Linus

Newsgroups: fa.linux.kernel
From: Larry McVoy <lm@bitmover.com>
Subject: coding style (was Re: [PATCH][2.5] UTF-8 support in console)
Original-Message-ID: <20030531144323.GA22810@work.bitmover.com>
Date: Sat, 31 May 2003 14:44:24 GMT
Message-ID: <fa.gg533br.1t42vpn@ifi.uio.no>

> Please linewrap after 80 chars.

Amen to that.

> +	if (!q) {
>
> Kill the blank line above.
>
> +		if (!q) return;
>
> Two lines again.

A couple of comments: in the BK source tree, we've diverged from the Linux
coding style a bit (maybe a lot, Linus has read the source, ask him).

One thing is

	unless (p) {
		....
	}
instead of
	if (!p) {
		....
	}

It's just a
#define unless(x) if (!(x))
but it makes some code read quite a bit easier.  I'm a stickler for not using
2 lines where one will do, i.e.,

	FILE	*f;

	...
	unless (f = fopen(file, "r")) {
		error handling;
		return (-1);
	}

You hiccup the first time you see it, then you can read it, then you
start using it.  Yeah, I know, I'm using the value of an assignment in
a conditional, trust me, it works fine.

One other one is the

	if (!q) return;

Chris said two lines, we don't do it that way.  The coding style we use is
a) one line is fine for a single statement.
b) in all other cases there are curly braces

	unless (q) return;	/* OK */
	unless (q) {		/* also OK */
		return;
	}
	unless (q)
		return;		/* not OK, no "}" */


The point of this style is twofold: save a line when the thing you are
doing is a singe statement, and make it easier for your eyes (or my
tired old eyes) to run over the code.  If you see indentation you know
it is a block and there will be a closing } without exception.

It keeps the line counts about 10% smaller or so in our source base.
If you are looking for bragging rights about how big your stuff is that
might be bad but I like it because I can read more code in a window.
--
---
Larry McVoy              lm at bitmover.com          http://www.bitmover.com/lm

Newsgroups: fa.linux.kernel
From: viro@parcelfarce.linux.theplanet.co.uk
Subject: Re: coding style (was Re: [PATCH][2.5] UTF-8 support in console)
Original-Message-ID: <20030531175632.GL9502@parcelfarce.linux.theplanet.co.uk>
Date: Sat, 31 May 2003 17:57:50 GMT
Message-ID: <fa.ngrug4j.1m763qn@ifi.uio.no>

On Sat, May 31, 2003 at 11:14:08AM -0600, Steven Cole wrote:

> statement when needed.
>
> return -ETOSENDERADDRESSUNKNOWN;	/* this is OK */
> return (value & ZORRO_MASK);		/* so is this */

Like hell it is.  Parenthesis are _not_ needed here - production is
<statement> -> return <expression> ;

The only messy '('-related case in C grammar is sizeof as unary operation
vs. sizeof ( <type> ) (lovely way to torture parsers and students on exam:
sizeof (int)*p).  Everything else is pretty straightforward...

Newsgroups: fa.linux.kernel
From: torvalds@transmeta.com (Linus Torvalds)
Subject: Re: Question about style when converting from K&R to ANSI C.
Original-Message-ID: <1054519757.161606@palladium.transmeta.com>
Date: Mon, 2 Jun 2003 02:11:01 GMT
Message-ID: <fa.k4n5a5f.1migg2t@ifi.uio.no>

In article <20030601132626.GA3012@work.bitmover.com>,
Larry McVoy  <lm@bitmover.com> wrote:
>On Sat, May 31, 2003 at 11:56:16PM -0600, Steven Cole wrote:
>> Proposed conversion:
>>
>> int foo(void)
>> {
>>    	/* body here */
>> }
>
>Sometimes it is nice to be able to see function names with a
>
>	grep '^[a-zA-Z].*(' *.c
>
>which is why I've always preferred
>
>int
>foo(void)
>{
>	/* body here */
>}

That makes no sense.

Do you write your normal variable definitions like

	int
	a,b,c;

too? No you don't, because that would be totally idiotic.

A function declaration is no different. The type of the function is very
important to the function itself (along with the arguments), and I
personally want to see _all_ of it when I grep for functions.

You should just do

	grep -i '^[a-z_ ]*(' *.c

and you'll get a nice function declaration with the standard kernel
coding style.

And I personally don't normally do "grep for random function
declarations", that just sounds like a contrived example.  I grep for
specific function names to find usage, and then it's _doubly_ important
to see that the return (and argument) types match and make sense.

So I definitely prefer all the arguments on the same line too, even if
that makes the line be closer to 100 chars than 80.  The zlib K&R->ANSI
conversion was a special case, and I'd be happy if somebody were to have
the energy to convert it all the way (which implies moving comments
around etc).

			Linus

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: [PATCH] fix for ide-scsi crash
Original-Message-ID: <Pine.LNX.4.58.0401200953470.2123@home.osdl.org>
Date: Tue, 20 Jan 2004 17:58:44 GMT
Message-ID: <fa.j4ckql4.1e2skgs@ifi.uio.no>

On Tue, 20 Jan 2004 Andries.Brouwer@cwi.nl wrote:
>
>     If Andries wants to
>     re-send the whitespace fixes, I can apply those too, but I hate applying
>     patches like this where the whitespace fixes hide the real fix.
>
> Yes, it seems we presently have no good mechanism / policy here.
> Patches are noise. If some kernel version works and another doesnt,
> one has to look at the diffs. Whitespace-only diffs are bad,
> I would never submit them. They also needlessly invalidate existing patches.

Whitespace-only diffs can be very useful. In particular, they are common
when somebody starts working on a piece of code without a maintainer, and
the old code was terminally broken wrt whitespace. Happens quite often in
the driver world.

So I don't have any real issues with applying whitespace-only patches, and
I much prefer them to patches that mix whitespace and bugfixes. In
particular, if the whitespace fixes are preparation for some other
cleanup, it's usually a good idea.

(I agree that if the whitespace fix is just random, it's usually not worth
it).

		Linus

From: Linus Torvalds <torvalds@osdl.org>
Newsgroups: fa.linux.kernel
Subject: Re: [patch 2.6.13-rc3] i386: clean up user_mode macros
Date: Mon, 25 Jul 2005 23:16:37 UTC
Message-ID: <fa.g3pv4bb.mgi6at@ifi.uio.no>
Original-Message-ID: <Pine.LNX.4.58.0507251608430.6074@g5.osdl.org>

On Mon, 25 Jul 2005, Chuck Ebbert wrote:
>
> Recent patches from the Xen group changed the X86 user_mode macros.
>
> This patch does the following:
>
>         1. Makes the new user_mode() return 0 or 1 (same as x86_64)

I _really_ prefer

	x != 0

over

	!!x

since double negation is not only a bad habit in natural languages, it's a
bad habit in computer languages too, for exactly the same reason. It's
confusing.

Ask a hundred random C programmers what "!!x" means, versus what "x != 0"
means, and time their replies.

I will bet you $5 USD that even if they all give the right answer (and I
suspect you'll get a few wrong answers in there too for the !! case),
they'll take a _lot_ longer answering the "!!x" version than they will the
"x != 0" question.

And guess what? That means that the "!!x" version is worse. It means that
people don't "see" what it means - they have to think about it. And you
shouldn't have to think about something like that, you should write it in
the obvious way in the first place.

		Linus

From: Theodore Tso <tytso@mit.edu>
Newsgroups: fa.linux.kernel
Subject: Re: A CodingStyle suggestion
Date: Sun, 04 Feb 2007 12:49:08 UTC
Message-ID: <fa.PZxGhxWoU6Ob/PPEzSDYHzMlVZQ@ifi.uio.no>

On Sat, Feb 03, 2007 at 01:59:51PM -0800, Randy Dunlap wrote:
> On Sat, 3 Feb 2007 23:58:48 +0200 Ahmed S. Darwish wrote:
> >
> > In CodingStyle Chapter 16 "Function return value and names", why not
> > adding a comment about the favorable community way of checking the return
> > value. ie:
> >
> > ret = do_method();
> > if (ret) {
> >    /* deal with error */
> > }
> >
> > and not other ways like:
> >
> > if (do_method()) or if ((ret = do_method()) > value) ...
> >
>
> I like it.  Please cc: Andrew Morton <akpm@osdl.org> on it.
> Hopefully he will merge it.
>

I'm going to have to disagree.  Sometimes if the main flow of the code
is down, it's actually better to do this:

	if ((err = do_foo()) < 0)
		return (err);
	if ((err = do_bar(current, filp)) < 0)
		return (err);
	if ((err = do_quux(filp, buffer)) < 0) {
		close(filp);
		return (err);
	}

Than to do something like this:

	err = do_foo();
	if (err < 0)
		return (err);
	err = do_bar(current, filp);
	if (err < 0)
		return (err);
	err = do_quux(filp, buffer);
	if (err < 0) {
		close(filp);
		return (err);
	}

The first is more concise, and it draws the reader's eye to what's
really going on.  The cleanup/return error path is less important, and
and it's pretty clear what's going on just from glancing at it.

						- Ted

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [5/6] 2.6.21-rc2: known regressions
Date: Tue, 06 Mar 2007 16:50:17 UTC
Message-ID: <fa.utaUvmrQRmc3VhuemtpyGUuhm8I@ifi.uio.no>

This is just a coding style thing, but I thought I should really point it
out, because these kinds of things quite often result in nasty bugs simply
because the source code is so hard to read properly:

On Tue, 6 Mar 2007, Ingo Molnar wrote:
>
> -static void hrtimer_switch_to_hres(void)
> +static int hrtimer_switch_to_hres(void)

Ok, so here's the quiz: does this function return "true on success, false
on failure", or does it return "zero on success, negative on failure"?

>  	if (base->hres_active)
> -		return;
> +		return 1;

Ahh, it must be "true on success", right?

>  	local_irq_save(flags);
>
>  	if (tick_init_highres()) {
>  		local_irq_restore(flags);
> -		return;
> +		return 0;

Ohh-oh! This is clearly a failure scenario! And indeed,
"tick_init_highres()" will do the "negative on failure, zero on success"
thing.

BUT! That means that you're testing the return value WRONG!

A function that returns a negative error value should be tested with

	if (tick_init_highres() < 0) {
		local_irq_restore(flags);
		return 0;
	}

because now you *see* that it's a failure.

So here's the coding style:

 - "true on success, false on failure" should be tested by just doing the
   implicit test against zero (because that's how C booleans work!)

   Example:

	if (everything_is_done())
		return;

   Or:

	if (!something_worked_ok()) {
		printk("Aiee! Bug!\n");
		return;
	}

 - "negative error values" should preferably always be tested as such

	if (tick_init_highres() < 0) {
		printk("Aieee! Couldn't init!\n");
		return 0;
	}

   or, much better, actually use a temporary variable called "err" or
   "error" or something, at which point "!error" is suddenly readable
   again:

	err = tick_init_highres();
	if (!err)
		return;

I know this sounds stupid, but we've long since come to the point where
source code readability on a *local* scale is damn important, simply
because that's how people look at code: they may not always remember
whether "zero is success" or "zero is false".

In general, I would suggest:

 - ALWAYS use "negative means error". If you had done that in this case,
   then hrtimer_switch_to_hres() would have been a lot more readable,
   *and* it could actually have returned the error code that it got to the
   caller. In general, it's just more information when you see

	error = some_function();
	if (error)
		return error;

   because even if it may generate basically *exactly* same code as the
   reversed "positive" version:

	if (!some_version_is_true())
		return 0;

   it simply has more semantic information for *humans*.

   And when you do this, *test it as such*. Either use an explicit "< 0"
   so that you *see* that you're testing an error value, or use that
   "retval/error = xyzzy()" pattern that is already showing "it's more
   than just true/false"

 - use "true/false" only for things where it's *really* obvious that the
   answer is never an error, and always a "was it true"?

Yeah, even so, the true/false kind of thing may be more common (especially
with small helper functions that are literally *designed* to be used just
as a conditional), but I think in this case, you really should have done
it as a "returns error" function. Partly because now it was throwing away
an error code, partly simply because in this case, it really wasn't about
true/false as much as about "did something error out and keep it from
succeeding?".

Maybe I'm just getting anal in my old age. I at one time tried to make
sparse check for these things, but there was no really sane thing I could
come up with (way way WAAY too much manual annotation).

I might have to break down and suggest people use

	bool somefunction(..)
	{
		if (... < 0)
			return false;
		...
		return true;
	}

just to (a) eventually have sparse check for these things but more
importantly (b) have people see more at a glance whether a function is
supposed to return "negative or success" or "true or false".

I've not generally been a huge fan of "boolean", especially in the
traditional C kind of sense (capital screaming letters, and really just an
"int" with lipstick). But with modern C, and "bool" defined as really
holding just 0/1 (in practice - "unsigned char"), we could actually check
these things (and verify with sparse that you never assign any integer
except for 0/1 to a boolean, and otherwise always have to use a real
boolean construct).

Thus endeth my overly long coding style rant.

		Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH -mm] 1/2: MMCONFIG: validate against ACPI motherboard
Date: Wed, 30 May 2007 04:45:41 UTC
Message-ID: <fa.igSq1eISes5hAoK5882gAdy9G2A@ifi.uio.no>

On Tue, 29 May 2007, Robert Hancock wrote:
>
> This path adds validation of the MMCONFIG table against the ACPI reserved
> motherboard resources.

Please fix the formatting of your code.

"for" and "if" are not functions, and they have a space before the
parenthesis.

And pretty much every single conditional in this patch is spread out over
two or more lines and has at least three different indentations. There's
something wrong here. Code can't look this bad and still be fine. Some of
this looks like random whitespace noise:

+               if(is_acpi_reserved(cfg->address,
+                                   cfg->address + size - 1))
+                       printk(KERN_NOTICE "PCI: MCFG area at %Lx reserved "
+                               "in ACPI motherboard resources\n",
+                               cfg->address);
+               else {

That's just horrid. Please try to make the code _look_ nicer.

For example, just making "is_acpi_reserved()" take a start/len thing
instead, would allow you to at least do

	if (is_acpi_reserved(cfg->address, size)) {
		printk(KERN_NOTICE "PCI: MCFG area at %Lx reserved "
			"in ACPI motherboard resources\n",
			cfg->address);
	} else {
		...

(and has the braces right too - don't pair an unbraced "if ()" with a
braced "else" statement), and that together with making the body of the
for-loop a separate function would possibly make that code read a lot
better.

Same goes for this thing:

+                       if((pci_probe & PCI_PROBE_CONF1) &&
+                          e820_all_mapped(cfg->address,
+                                          cfg->address + size - 1,
+                                          E820_RESERVED))
+                               printk(KERN_NOTICE "PCI: MCFG area at %Lx reserved in E820\n",
+                                       cfg->address);
+                       else
+                               goto reject;

there really is *not* a highly coveted prize for having the most different
indentation in the fewest possible lines of code!

Yeah, I realize that maybe this is nit-picking, but trying to read this
patch really does make you go blind. It violates so many coding standards
that it's almost impossible to read the code itself. It's made worse by
the fact that you then also used Thunderbird to send the patch, and had it
set for

	Content-type: text/plain; charset=ISO-8859-1; format=flowed

where that "format=flowed" means that basically no mail-client will be
able to read it sanely (because a lot of them will re-flow the text), and
you have to save it to a file before you can even comment on it.

Gaah. See

	http://mbligh.org/linuxdocs/Email/Clients/Thunderbird

where the most important sentence is "Thunderbird is written by aged whore
monkeys stoned on crack". But it also talks about how to disable that
idiotic format=flowed for patches, and how to make sure it's not wrapping.

But as mentioned, your patch itself had some whitespace issues even aside
from the regular Thunderbird breakage.

		Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH -mm] 1/2: MMCONFIG: validate against ACPI motherboard
Date: Wed, 30 May 2007 17:38:39 UTC
Message-ID: <fa.FTwSRO1y148pfPjVm6AnBUvdPlQ@ifi.uio.no>

On Wed, 30 May 2007, Dave Jones wrote:

> I'm convinced there's some contest to see who can make the worst
> graphical mail client for Linux. I'm not sure what the prize is,
> or who's winning, but the entries so far are horrific.

LOL.

Although in this case it was actually not a Linux MUA:

	User-Agent: Thunderbird 2.0.0.0 (Windows/20070326)

but I didn't want to embarrass Robert publicly before you brought up the
issue of operating systems ;)

Your procmail filter looks somewhat broken, btw. It will trigger on things
that aren't headers, and it will potentially _not_ trigger on headers (ie
if they are line continuations). But I guess it works well enough in
practice.

			Linus

From: Al Viro <viro@ftp.linux.org.uk>
Newsgroups: fa.linux.kernel
Subject: Re: [RFC] Documentation/CodingStyle: Add rules for goto labels
Date: Sun, 03 Jun 2007 14:30:03 UTC
Message-ID: <fa.1nVUr+8+xLzSXfU7FSI80pfgylE@ifi.uio.no>

On Sun, Jun 03, 2007 at 10:24:50PM +0800, WANG Cong wrote:

> +Do care when you use Lindent to indent your code, since it may use spaces
> +instead of tabs before a goto label and it may also align the label in a
> +wrong position. A goto label should be aligned in the column that is 8
> +characters ahead of the statement just below this label. Please fix it manually
> +if you find Lindent is wrong.

Lindent is wrong, but the style you are advocating is, at the very
least, not universal.  Equally (if not more) common is putting label
in column 1, period.  Regardless of indentation level of the statement
following it.

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: coding style
Date: Fri, 15 Jun 2007 20:22:21 UTC
Message-ID: <fa.q/y+OgIo0TLc9kPZ6RQdg3lZxPw@ifi.uio.no>

On Fri, 15 Jun 2007, Cyrill Gorcunov wrote:
> |
> | 	from CodingStyle:
> | 	Tabs are 8 characters, and thus indentations are also 8
> | 	characters.  There are heretic movements that try to make
> | 	indentations 4 (or even 2!) characters deep, and that is akin
> | 	to trying to define the value of PI to be 3.
> |
> | Linus (did he wrote that part?) and the heretics both can have their fun
> | without impacting each other. If we wanted to force the user to have
> | exactly 8 screen blanks, we should use spaces throughout.

I did indeed write that.

Tabs are 8 characters in the kernel coding style.

And yes, I also wrote the other quote:

> Dunno who wrote that part :(. Jan, look:
>
> 	Now, some people will claim that having 8-character indentations makes
> 	the code move too far to the right, and makes it hard to read on a
> 	80-character terminal screen.  The answer to that is that if you need
> 	more than 3 levels of indentation, you're screwed anyway, and should fix
> 	your program.

and I think that's in many ways even more important than the 8-character
tab, because deep indentation is unreadable even if you *can* fit it on a
single line.

In the kernel, we try to split functions up, and perhaps use inline
functions etc, and really really avoid deep indentation.

		Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: coding style
Date: Fri, 15 Jun 2007 20:40:37 UTC
Message-ID: <fa.MQ4ZJT+8bwUXKUz57iGPTGqMuj0@ifi.uio.no>

On Fri, 15 Jun 2007, Jan Engelhardt wrote:
>
> Linux maintainers will enforce \t "being"[1] 8, and will also enforce
> the 80-column limit[2].

Heh. Actually, Linux maintainers have generally very consciously _avoided_
trying to "enforce" coding style issues.

We encourage. Sometimes people actually end up running "lindent" on code
and enforce the rules through that, but that is actually fairly rare, and
most of the time that happens only when somebody decides to tackle some
subsystem that hasn't effectively been maintained in a while, and in order
to do cleanups, the person needs to just make it readable first.

Is that the _only_ case when we do that? No. There's the occasional
Lindent run done randomly, usually brought on by something _really_
atrocious, but I would say that generally we try to keep code churn due to
_just_ coding style issues to a minimum.

I do sometimes end up not taking *new* code if it is really really ugly.
So it definitely happens. But...

			Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: coding style
Date: Sat, 16 Jun 2007 15:50:44 UTC
Message-ID: <fa.6J7/kVSpQCiq9zosXO/srC7b+rw@ifi.uio.no>

On Sat, 16 Jun 2007, Jan Engelhardt wrote:
> >
> >Heh. Actually, Linux maintainers have generally very consciously _avoided_
> >trying to "enforce" coding style issues.
>
> Really? "it's not going to be merged unless you turn all uint32_t into
> u_int32_t" is a paraphrased variant of what I was told this month.

I suspect different maintainers are hung up on different things, so yes,
certain things are more likely to carry red flags, and it also depends on
the patch.

For example, if I get a patch for something that is a whole driver, I
generally think that while I *prefer* to see it follow the kernel coding
style, I also expect that the person who sends me the driver is the one
who is going to maintain it in the future, and thus his personal coding
style preferences will override any but the strongest objections.

(So if somebody sends me a FSF-style "tabs are two characters, and
functions must be longer than 300 lines" mess, I generally would prefer to
not take it at all, but for some really obscure driver I might not care).

In contrast, if a patch modifies code that somebody else really will end
up touching in the future (maybe not "maintain", but maybe there is no
single and obvious maintainer), it had better match the code around it.

So to take your particular example: For me, "uint32_t" is certainly better
than "u_int32_t" (and there's seven times as many of the former as the
latter in the kernel), but for code _I_ would touch, I'd actually prefer
the Linux internal "__u32"/"u32", which have no question about what their
user-space visibility is (ie "__u32" is *always* ok in a header file that
is visible to user space).

But would I make it a huge issue? Not personally. So it will depend on the
maintainer.

(Personally, I think the "small functions, no deep levels of indentation,
and tabs are 8 characters wide" are the most important part by far. But I
do actually end up complaining about function naming etc too).

		Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [RFC PATCH 5/5 v2] Convert tasklets to work queues
Date: Sat, 23 Jun 2007 18:26:30 UTC
Message-ID: <fa.vHCh4iCtBaGDS3DKFFUvniEh9gM@ifi.uio.no>

On Sat, 23 Jun 2007, Andrew Morton wrote:
>
> Anyway.  Please fix the many correct warnings which checkpatch.pl
> generates

Actually, please don't.

Especially for code movement, *just* do the movement. Screw any
checkpatch.pl crap - the code is better off not changing, because that way
a big patch can not only be proven to not change anything at all, but
software archeology tools can trivially find the true history of the code
over code movement.

For example, "git blame -C" already finds copies and can annotate the
history of a line of code past a pure code movement. But if you move *and*
change things at the same time, it gets a lot harder to show where the
code came from and that the movement itself caused no regressions.

So do cleanups _separately_ from movement.

(Yeah, yeah, "git blame -C -w" will generally work across whitespace
changes too, but only whitespace _within_ a line. If you do things like
split long lines etc, you immediately have a lot harder time to follow
these things. Not impossible, but the point is that you're not *fixing*
anything, you're just making things *worse* by doing changes and code
movement at the same time).

Quite frankly, I personally am considering removing "checkpatch.pl". That
thing is just a nazi dream. That hard-coded 80-character limit etc is just
bad taste.

Dammit, code cleanliness is not about "automated and mindless slavish
following of rules". A process that is too inflexible is a *bad* process.
I'd much rather have a few 80+ character lines than stupid and unreadable
line wrapping just because the line hit 87 characters in length.

I don't have 25 lines on a screen either.

		Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [RFC PATCH 5/5 v2] Convert tasklets to work queues
Date: Sat, 23 Jun 2007 19:25:29 UTC
Message-ID: <fa.Fg/QtxFYoXoaOhDwB+2XxoTFSH0@ifi.uio.no>

On Sat, 23 Jun 2007, Randy Dunlap wrote:
> >
> > Quite frankly, I personally am considering removing "checkpatch.pl". That
> > thing is just a nazi dream. That hard-coded 80-character limit etc is just
> > bad taste.
>
> Who wrote that part of CodingStyle?

I don't think that the notion of "80 characters per line" is *wrong* per
se.

So don't take this the wrong way - I think the goal really _should_ be
that a function fits on a vt100 terminal window (*both* ways: less than 24
lines of code, and less than 80 characters wide).

So I think the notion of trying to keep lines below 80 characters is
admirable. That part isn't the problem.

The problem is when "coding style guidelines" become "hard inviolate
rules".

Yes, code should be less than 80 characters wide.

But hey, sometimes it's just more readable to have one line that is
slightly longer than it should be, than to split something that is awkward
to split.

The thing that scripts (and computers) have a really hard time with is
"judgement".

So I don't disagree with any of the checkpatch.pl things individually, but
I do disagree with the notion that we should enforce strict rules, when
all the guidelines are really just guidelines.

For example, the coding style also says that you should avoid indentation
that is more than three deep. It's _true_, but does that mean that we
should make deeply indented code *illegal*? Obviously not. It's a "please
try to split your functions up" kind of thing.  Not a hard rule.

The only reason I picked the 80-character thing in particular is that
I've seen a fair number of patches that don't do anything *but* change
that, and quite often I think it makes the code look worse if it's not
done judiciously..

There are other things we really *could* be strict against. For example,
the "space followed by tab" thing really is something where a strict rule
(at least for C files) probably really *is* a good idea, because there
really is never a really good reason for it. Add the fact that the problem
is "invisible" in most editors, and having a script that checks for it
makes even more sense.

So I'm happy with checkpatch.pl as a *guide*, but I'm not happy if it
means that people start taking it as a *requirement*.

(And then I'm doubly unhappy when there are issues like code movement and
file renames etc, where there are other reasons why we actually want to
keep the code as unmodified as possible).

		Linus

From: Al Viro <viro@ftp.linux.org.uk>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH 06/68] 0 -> NULL, for arch/frv
Date: Fri, 27 Jul 2007 10:41:19 UTC
Message-ID: <fa./R9OBPteI0izLjAidAiPJMNml3Y@ifi.uio.no>

On Fri, Jul 27, 2007 at 12:21:53PM +0200, Yoann Padioleau wrote:
> > On Fri, Jul 27, 2007 at 11:44:35AM +0200, Yoann Padioleau wrote:
> >>  	pte = pte_alloc_kernel(pme, va);
> >> -	if (pte != 0) {
> >> +	if (pte != NULL) {

> I don't understand. pte is a pointer right ? So why should we
> keep the == 0 ?

Idiomatic form for "has allocation succeeded?" is neither "if (p != 0)" nor
"if (p != NULL)".  It's simply "if (p)".

Note that it depends upon context.  For something that combines assignment
with test
	if ((p = foo_alloc()) != NULL)
would be the right way to go.  Ditto for
	flag = (p == NULL)
(alternative would be "flag = !p", which is usually not nice or even
"flag = !!p" for the opposite test, and that's bloody atrocious).

For places like
-      if (spu_disassemble_table[o] == 0)
+      if (spu_disassemble_table[o] == NULL)
        spu_disassemble_table[o] = &spu_opcodes[i];
it's a matter of taste; there I'd go for explicit comparison with NULL.
I'd also go for explicit comparisons in places like
-               wait_event(journal->j_wait_done_commit, journal->j_task == 0);
+               wait_event(journal->j_wait_done_commit, journal->j_task == NULL);

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH] 0/3 coding standards documentation/code updates
Date: Sat, 29 Sep 2007 18:19:29 UTC
Message-ID: <fa.GEVx+xd29mVVLTpw4gl/Z4MW9gM@ifi.uio.no>

On Fri, 28 Sep 2007, Erez Zadok wrote:
>
>  Documentation/CodingStyle         |   88 +++++++++++++++++++++++++++++++++++++-

I'm not very happy with this.

"CodingStyle" should be about the big issues, not about details. Yes,
we've messed that up over the years, but let's not continue that.

In other words, I'd suggest *removing* lines from CodingStyle, not adding
them. The file has already gone from a "good general principles" to "lots
of stupid details". Let's not make it worse.

			Linus

From: Theodore Tso <tytso@mit.edu>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH] 0/3 coding standards documentation/code updates
Date: Sun, 30 Sep 2007 02:08:06 UTC
Message-ID: <fa.vKyGGNe5n3ddcFlTNB0x5vV7Vn4@ifi.uio.no>

On Sat, Sep 29, 2007 at 03:56:38PM -0400, J. Bruce Fields wrote:
> It'd be a start just to revert CodingStyle to its original content and
> move the rest to CodingStyleReference.  But someone would want to skim
> through the CodingStyle history for any legimate corrections that we
> want to keep.

How about CodingStyleSuggestions?  And let's make it clear they are
suggestions, and not things that checkpatch should be flaming about
unless people request the all of the annoying associated false
positives via --spam-me-harder.  :-)

       	      	      	       			  - Ted

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH] 0/3 coding standards documentation/code updates
Date: Sun, 30 Sep 2007 03:01:26 UTC
Message-ID: <fa.BS1N4OQAMAh1lrDdFKMOSkAH33U@ifi.uio.no>

On Sat, 29 Sep 2007, Valdis.Kletnieks@vt.edu wrote:
>
> I think there needs to be a "sense of fairness" attached here - CodingStyle
> should cover all the stuff maintainers/reviewers are allowed to whinge about.

No.

People whine too much as is. Don't give them *license* to do so.

		Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH] 0/3 coding standards documentation/code updates
Date: Sun, 30 Sep 2007 03:36:46 UTC
Message-ID: <fa.HmFdnNFkBh2gpfyUX3UE6ihWMMg@ifi.uio.no>

On Sat, 29 Sep 2007, Valdis.Kletnieks@vt.edu wrote:
>
> I kind of meant the *maintainers* couldn't whine about things not in
> CodingStyle ;)

No, I understood you.

I'm personally of the opinion that the automated style checking, and
having detailed rules is always a mistake.

It's *much* better to teach people the big things. And the small things
will vary from person to person _anyway_, so it's not like you can really
document them.

		Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH 1/1] 2.6.24, net/bluetooth/hci_sysfs.c, cleaned code
Date: Sun, 27 Jan 2008 07:00:41 UTC
Message-ID: <fa.L4LEpR1enMrOp8dhR1mg/gvLVYk@ifi.uio.no>

On Sun, 27 Jan 2008, Michael K?hn wrote:
>
> the attached Patch will remove a compiler warning in
> net/bluetooth/hci_sysfs.c:

Please don't do this. I realize that gcc suggests just adding parenthesis,
and yes, it makes the warning go away, but it's not sensible from a human
semantic standpoint.

So if the warning bothers you, please change

	while (dev = device_find_child(..)) {

into something like

	while ((dev = device_find_child(..)) != NULL) {

which also gets rid of the warning, but now does it with something that
makes sense to a human too.

Just arbitrarily adding another set of parentheses never made much sense
to me. It's semantically still the exact same expression, just the fact
that gcc suddenly doesn't warn about the assignment because it's "hidden"
by the extra parenthesis is just odd.

		Linus

From: Al Viro <viro@ZenIV.linux.org.uk>
Newsgroups: fa.linux.kernel
Subject: Re: Merging of completely unreviewed drivers
Date: Fri, 22 Feb 2008 03:13:53 UTC
Message-ID: <fa.fCX12r/TKCdBbg58GaqQFmlcyec@ifi.uio.no>

On Fri, Feb 22, 2008 at 03:23:45AM +0100, Krzysztof Halasa wrote:
> Al Viro <viro@ZenIV.linux.org.uk> writes:
>
> > ... if your style is lousy.  I agree that situation with printks is
> > not normal in that respect and I certainly have no love for the
> > checkpatch nonsense, but pressure to keep the fucking nesting depth
> > low is a Good Thing(tm).
>
> Indeed. Unfortunately it is orthogonal to the line length limit.

Not quite.  Add such things as choice of sane identifiers.  And sane use of
local variables, while we are at it - things like twenty lines of
	foobar[(index + 1) % BLAH]->spork.vomit[12]->field_name = <expr>;
with the only difference in the field_name, except for one line where
we have a typo and see 11 instead of intended 12, are responsible for quite
a few of such overruns.

IMO the line length overruns make good warnings.  Not as in "here's a cheap
way to get more changesets", but as in "that code might have other problems
nearby" kind of heuristics.

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: Merging of completely unreviewed drivers
Date: Fri, 22 Feb 2008 03:14:37 UTC
Message-ID: <fa.oeIrFrOqrCO2E0wgd1HGDWcfDus@ifi.uio.no>

On Fri, 22 Feb 2008, Al Viro wrote:
>
> ... if your style is lousy.  I agree that situation with printks is
> not normal in that respect and I certainly have no love for the
> checkpatch nonsense, but pressure to keep the fucking nesting depth
> low is a Good Thing(tm).

I do agree, but that has little to do with line length *directly*.

IOW, I'd personally be happier with a checkpatch that calculated
"complexity" and indentation over line length.

There is definitely a correlation there: there is no question that complex
lines with deep indentation tend to be long. So yes, "long lines are
correlated with bad code" is certainly true to some degree.

But sometimes lines are long just because it's a function call with
multiple parameters, and it's just three levels indented, and it had a
string there too. It may be long, but it's not complex, and keeping it on
one line actually makes it much easier to visually parse (and grep for,
for that matter).

So I'd be happier with warnings about deep indentation (but how do you
count it? Will people then try to fake things out by using 4-space indents
and then "deep" indentations will look like just a couple of tabs?) and
against complex expressions (ie "if ((a = xyz()) == NULL) .." should just
be split up into "a = xyz(); if (!a) ..", but there are sometimes reasons
for those things too!

		Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: Merging of completely unreviewed drivers
Date: Sat, 23 Feb 2008 17:35:04 UTC
Message-ID: <fa.tVXbasmAm/qgo3m+ZdQ7Y3Dv6jQ@ifi.uio.no>

On Sat, 23 Feb 2008, David Newall wrote:
>
> Do you actually get 80 columns wide on it?

Do people really care that deeply?

I still sometimes use small terminal windows - for a while I had my
default terminal come up as 100x40, but I'm back to the standard 80x24,
and while I often resize them, I certainly don't always.

And do I find lines longer than 80 charactes unreadable? Hell no.

Quite frankly, on a 80x24 display, I'll take long lines over split-up ones
*any* day. For things like doing "git grep xyzzy", I'd *much* rather get
the occasional long line that wraps (or, if I'm in "less -S", that I have
to press right-arrow to see), than see just a meaningless fragment because
somebody decided that they should always fit in 80 characters.

So *consistently* long lines are the problem, not the occasional one. The
occasional one is likely more readable as it is, than split up.

Here's an example from code that actually looks pretty good in general:

	static unsigned long
	calc_delta_mine(unsigned long delta_exec, unsigned long weight,
	                struct load_weight *lw)

and look around that function in general: it's doesn't match the coding
standard, but also compare the output of

	git grep calc_delta_mine

with the output of something like

	git grep update_load_sub

which actually shows you what the calling convention is.

So putting that long function definition on one line would make it a
108-character line or something like that, but it would have advantages
too.  It would have advantages for anything that is line-based (I use
grep for *everything*, but maybe I'm just odd), but it would also
actually be more readable for the people who have bigger windows.

But my point is, some of those advantages remain even with small
terminals, and quite often the downsides aren't even all that huge.
Most editors wrap or chop the line according to your preferences (mine
are personally to chop), and if it's a fairly uncommon thing, those
downsides shrink further.

Is 108 characters perhaps *too* long? In the above case it probably is,
since the downside of splitting the patch is pretty small (it's a static
function, only used in that file, the "grep" argument is weak, yadda
yadda).  But I'm just saying that it's not 100% obvious *even*if* you're
on a 80x24 terminal, and in some other cases the downside of splitting
the line can be much bigger (strings or more spread-out function calls
and declarations etc).

The line length problem would probably be better attacked as something
more akin to the rule

 - do a rolling window of <n> last non-empty lines (n ~ 15 or so)

 - if more than <m> of those lines were longer than 72 charactes,
   something is wrong (m ~ 5 or so).

which talks more about what matters - too deep indentation. And also
attacks the problem that is really relevant: it's that kind of code that
ends up being unreadable because so *much* of it is cut off or wrapped.

			Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: Merging of completely unreviewed drivers
Date: Sun, 24 Feb 2008 04:51:00 UTC
Message-ID: <fa.uPZfFrnhVRGeo6pTvbmHrKOFAfw@ifi.uio.no>

On Sun, 24 Feb 2008, David Newall wrote:
>
> > which talks more about what matters - too deep indentation.
>
> What's too deep?  Is the following too deep?

It would be, if it weren't artificially so, for violates several kernel
coding standards, one being that the "case" labels indent with the switch,
not under it (the other being the placement of braces).

> (Yes, I know, "we don't indent 'case' because it consumes too much
> room."

No, that's not it at all. We don't indent 'case' because it matches with
the 'switch', not because of any room issues.

> That's inconsistent with the rest of normal indenting style, and
> a poor excuse to keep within an obsolete and unnecessary restriction.)

It's not at all inconsistent. It's just making clear how the parts of the
function group together.

Indenting a case-statement an extra level is as stupid as indenting "else"
one extra level from the "if ()" it goes together with. Do you think that
would be sane?

The fact that the 'case' thing is technically parsed as a separate
statement in C doesn't change anything.

		Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [git pull] PCI pull request for 2.6.27
Date: Wed, 16 Jul 2008 23:51:24 UTC
Message-ID: <fa.zduc1uCUeIrFLZNg6R1xFa4SAZc@ifi.uio.no>

On Wed, 16 Jul 2008, Jesse Barnes wrote:
>
> Miklos Vajna (1):
>       x86/PCI: janitor work in irq.c

Please don't take patches like this.

If it's janitor work, the end result should be better. But it's not. This
patch is full of stuff like

-	for(addr = (u8 *) __va(0xf0000); addr < (u8 *) __va(0x100000); addr += 16) {
+	for (addr = (u8 *) __va(0xf0000); addr < (u8 *) __va(0x100000);
+		addr += 16) {
 		rt = pirq_check_routing_table(addr);

Which just brings negative value. The code is _harder_ to look at, not
easier.

The 80-character limit is less important than making code look obvious and
indentation being readable. Splitting the for(;;) loop just made the
indentation look like total crap.

I'm fixing it up (since it also caused trivial conflicts), but I'd ask
people to just ignore that sh*t-for-brains that is the long-line warning
when trying to fix it may silence a warning, but results in worse code!

		Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [git pull] PCI pull request for 2.6.27
Date: Thu, 17 Jul 2008 00:38:35 UTC
Message-ID: <fa.78lgwB+pmHagKwEFQB9JF5Xpyw0@ifi.uio.no>

On Thu, 17 Jul 2008, Maciej W. Rozycki wrote:
>
>  Conveniently "for" is short enough for indentation like this:
>
> 	for (addr = (u8 *) __va(0xf0000);
> 	     addr < (u8 *) __va(0x100000);
> 	     addr += 16) {
>                 rt = pirq_check_routing_table(addr);

I don't actually like that one very much either.

It's perfectly readable when looking at things closely, but it's not very
nice when quickly "scanning" code visually. It looks like two separate
indents.

Btw, that "code scanning" is not necessarily a bad idea. It's actually
pretty interesting to print code out in a 2-point font (or just open a
terminal and do "ctrl -" several times to make the code basically
unreadable). See if the code flow makes sense from 10,000 feet - you can
pick up overlong functions and various other dubious practices really
clearly (#ifdef's in code etc).

(IOW, the whole point of the exercise is to _not_ be able to actually read
the code, but just look at the _shape_ of it).

Btw, that commit also did things like change the coding style to a
non-kernel coding style by changing

	static int function(xyz..)

to

	static int
	function(xyz..)

just to make lines shorter. Again - introducing bigger problems than it
actually fixes.

				Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH 02/27] drivers/net: fix sparse warnings: make do-while
Date: Tue, 23 Dec 2008 18:09:51 UTC
Message-ID: <fa.CC5IndY4ebq4YzmmsVp93qvOt4Q@ifi.uio.no>

On Tue, 23 Dec 2008, Krzysztof Halasa wrote:

> Harvey Harrison <harvey.harrison@gmail.com> writes:
>
> >> There are many ways to make the code more merge friendly at a cost of
> >> readability. Hope we don't go this way.
> >
> > Linus himself added that particular warning to sparse...may want to check
> > with him the reason for it.
>
> Once again, this is a personal thing, and a harmless one.

It's more than that. I added the check after some person who had been
programming the kernel (and thus was supposedly fluent in C) literally
could not parse a macro that had "do x while (y)" in it.

Why? Because it's so uncommon, and because "while (y)" on its own means
something totally different.

So the syntactic sugar to _always_ have do-while loops have that brace is
a way to avoid one of the rather few places where the C language has
syntax that is very context-dependent.

Another example of this is "sizeof". The kernel universally (I hope) has
parenthesis around the sizeof argument, even though it's clearly not
required by the C language.

It's a coding standard.

And quite frankly, anybody who works on gcc has no place complaining about
sparse coding standard warnings. They are a _hell_ of a lot better than
some of the really crazy warnings gcc spews out with "-W". At least the
sparse warnings you can make go away while making the code more
understandable. Some of the -W warnings are unfixable without breaking the
source code.

		Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH 02/27] drivers/net: fix sparse warnings: make do-while
Date: Tue, 23 Dec 2008 23:38:30 UTC
Message-ID: <fa.pgIS9xWbhDccOIHMR1WlINiTW+Q@ifi.uio.no>

On Wed, 24 Dec 2008, Krzysztof Halasa wrote:
>
> So is a case like
> 	do
> 		x;
> 	while (y);
> It can't be made more clear with brackets.

Oh yes it can. People look at that, and it's so uncommon that they
literally believe it is a mis-indent.

Your example with nested if-statements are totally pointless, because you
didn't even apparently understand my comment about "while()" having two
totally different meanings (which is not true of "if()"), nor realize the
importance of how common something is.

Common patterns become things that people take for granted and don't have
any trouble with. In contrast, do-while without braces is _extremely_
uncommon.

> IOW: improving the style is great. Changing it only to silence some
> tool is not.

Sorry, you're wrong. It's not changed to silence some tool. THIS IS THE
KERNEL CODING STYLE.

I don't care one whit about your personal coding style. The kernel has
braces. End of discussion. sparse complains about lack of them.
Comprende?

> Right, but they (at least for me) make it more readable.

I don't care.

> kmalloc(sizeof i) just doesn't look good, the operator looks like
> a variable name.

And I agree with you. "sizeof i" doesn't look good. It's uncommon, and
doesn't match peoples expectations.

> But there is this return statement. Some people tend to write
> return (x); I simply write return x;

I do to. And it's the common thing to do, and only totally confused people
think that 'return' is a somehow remotely like a "function" of its
arguments (the way 'sizeof' is - sizeof _is_ a function of its arguments,
albeit a very rare one).

> It's clear, and so is a simple do-while.

No. "return x;" is clear because it's the common thing, which means that
peopel are good at reading it.

Another example of "common vs non-common" is this:

	if (0 <= x)
		do something..

is something that crazy people do (sadly, one of the crazy people taught
the git maintainer C programming, so now even sane people do it). It's
crazy because it's uncommon, which means that most people have to think
about it A LOT MORE than about

	if (x >= 0)
		do something..

even though technically both are obviously EXACTLY THE SAME THING.

Can you see the argument? Doing things the common way is important,
because it allows people to see what they mean without having to think
about it. They just scan it, and the meaning is clear.

And that's why "do while" without braces is bad. If you scan it quickly on
its own, you may well end up just seeing the

	while (x);

part, and get confused ("oh, a delay loop"). But if you see

	} while (x);

you aren't confused, because the latter one is clearly an ending condition
of a do-while loop, IN A WAY THAT THE FIRST ONE IS NOT!

See?

do-while is very special, because as mentioned, "while" is a really magic
C keyword that has two TOTALLY DIFFERENT meanings. Don't make people look
for the "do".

		Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [git pull] x86 fixes
Date: Mon, 12 Jan 2009 19:48:22 UTC
Message-ID: <fa.qWEjWTRWpCDgUe4yJ7N8ARnu1J4@ifi.uio.no>

On Mon, 12 Jan 2009, Pallipadi, Venkatesh wrote:
> +		if (strict_prot ||
> +		    (want_flags == _PAGE_CACHE_UC_MINUS &&
> +		     flags == _PAGE_CACHE_WB) ||
> +		    (want_flags == _PAGE_CACHE_WC &&
> +		     flags == _PAGE_CACHE_WB)) {

Please don't write code like this.

Do it as an inline function that returns true/false and has comments on
what the hell is going on.

If a conditional doesn't fit on one line, it should generally be
abstracted away into a readable function where the name explains what it
does conceptually.

		Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH] ptrace: checkpatch fixes
Date: Wed, 08 Apr 2009 17:21:27 UTC
Message-ID: <fa.CJpLjQC2tjrZaMXMtdhXZtK0/jE@ifi.uio.no>

On Tue, 7 Apr 2009, Roland McGrath wrote:
>
> This fixes all the checkpatch --file complaints about kernel/ptrace.c
> and also removes an unused #include.  I've verified that there are no
> changes to the compiled code on x86_64.

Please don't bother with that insane "line length" option when using
"--file". At least not if the "fix" is to just mindlessly split the line.
That is _never_ a fix.

Changes like these:

> -int ptrace_readdata(struct task_struct *tsk, unsigned long src, char __user *dst, int len)
> +int ptrace_readdata(struct task_struct *tsk, unsigned long src,
> +		    char __user *dst, int len)

> -int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long dst, int len)
> +int ptrace_writedata(struct task_struct *tsk, char __user *src,
> +		     unsigned long dst, int len)

>  	case PTRACE_GETEVENTMSG:
> -		ret = put_user(child->ptrace_message, (unsigned long __user *) data);
> +		ret = put_user(child->ptrace_message,
> +			       (unsigned long __user *) data);

just make the code harder to 'grep'.

Yes, at some point you have to split lines, but that point is not 80
columns any more.  The advantage of getting the whole line when grepping
for function names much outweighs the downside of somebody using those
old 80x24 green phosphorous vt52's.

[ The same thing very much goes for complex if-statements etc.  If
  people can't stand the long lines, the primary solution would be to
  turn a complex conditional into a helper inline functions, or to fix
  excessive indentation by splitting up functions.

  In the above case, the last one could perhaps have been handled
  creating a new variable for and moving the cast to the initialiser,
  for example. Is it worth it to avoid a 85-column line? Probably not.

  And some lines just end up long.  I think 100 characters may be a
  more reasonable limit for "too long", but quite frankly, it depends on
  the line.

  So I think 'checkpatch' is pure crap in this area, and I've told
  people so before, and they keep telling me that it has relaxed it's
  idiotic warnings, but that is apparently just a lie. ]

Oh well.  If I actually read perl, I could parse what the hell those
80-character rules are in checkpath.  It already has random "it's ok if
X" stuff.  But it never seems to really have any "oh, but splitting is
worse" logic.

			Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH] ptrace: checkpatch fixes
Date: Wed, 08 Apr 2009 20:47:26 UTC
Message-ID: <fa.1cOBTztDRPQExCUuGbmEvwtQnQE@ifi.uio.no>

On Wed, 8 Apr 2009, Christian Borntraeger wrote:
>
> Isnt checkpatch just following what is written down in the Documentation
> folder? Maybe adopting the following part of CodingStyle and add more
> examples for good and bad would give the checkpatch authors a better
> idea about your intent.

The thing is, it's true that it's good if things fit in 80 columns.

But _splitting_ lines isn't the answer. Making code simpler is, but
somehow the 80-column warning never causes that to happen - instead people
just split.

And yes, I guess we should remove the language saying so. It's not from
my original coding stule, it was added later by others, and came through
Andrew (commit 560362dafe4de60db70f2c298a53f4613453a78b: "[PATCH]
Codingstyle update" in the historical Linux archive).

			Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH] ptrace: checkpatch fixes
Date: Thu, 09 Apr 2009 15:03:56 UTC
Message-ID: <fa.5AbrObQXXsV9b+elqSWnHJqEdUU@ifi.uio.no>

On Thu, 9 Apr 2009, Ingo Molnar wrote:
>
> We should perhaps introduce an too-deep-indentation warning: any
> function with "[;{}]$" lines of 4 tabs in a row is already suspect
> IMHO. At 5 it's definitely crazy and ugly.
>
> This would be a very efficient function-length reductor: it cannot
> be worked around via line wraps.

People would start using spaces to try to work around it instead, which is
a worse cure than the problem.

Also, the thing is, a long _individual_ line is not a problem even if you
have a 80-column terminal. Sane editors will have a marker for "this line
continues", and even if you have an insane editors that doesn't do that,
it's pretty obvious - and if you really care about the end of that
_particular_ line (most of the time you don't), you can just move to that
line.

So if you have a couple of long lines occasionally, that's not a huge
problem. In fact, that's why I hate splitting lines so much: the "false
indentation" that a line split causes is generally much more confusing
visually (not so much in something like a function header, but often very
much so inside the code itself).

> It would also be wonderful to warn about bad 80 columns 'fixes' -
> i've seen way too many perfectly fine cleanups damaged by ugly
> line-wrapping solutions.

The thing is, it's very hard to warn about those. You need more
understanding than your average perl-script can ever get.

> We could also up the limit to 90 or 100 columns. My terminals are at
> 90 columns and that's still pretty ergonomic.

I tend to start out with a 80x24 and just resize it, and end up at some
random value. It's usually in the 90x40 range for me. But I do want the
code to be perfectly _readable_ in a 80x24 window, and quite frankly, if
you look at something like kernel/ptrace.c, it really generally is.

So sure, that "int ptrace_readdata()" line is longer than that, and won't
show completely. But you don't miss any huge glaring code issues even in
the truncated mode. In fact, if I try to use 80x24, my biggest issue will
inevitably be not the 80 part, but the 24 part.

IOW, I think there is much more reason to hate long _functions_ than there
is reason to hate long lines. Both cause you to scroll. The long function
where there is action over more than 24 lines happens a lot more.

					Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH 00/30] C/R OpenVZ/Virtuozzo style
Date: Mon, 13 Apr 2009 18:47:40 UTC
Message-ID: <fa.U0+kr72allIliCpAyU3HvCIlflk@ifi.uio.no>

On Mon, 13 Apr 2009, Alexey Dobriyan wrote:
>
> Well, in OpenVZ everything is in kernel/cpt/ and prefixed with "cpt_"
> and "rst_".

So?

We're not merging OpenVZ code _either_.

> And I think "cr_" is super nice prefix: it's short, it's C-like,
> it reminds about restart part

It does no such thing. THAT'S THE POINT. "cr" means _nothing_ to anybody
else than some CR-specific people, and those people don't even need it!

Look around you. We try to use nicer names. We spell out "cpufreq", we
don't call it "cf".

		Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH 00/30] C/R OpenVZ/Virtuozzo style
Date: Tue, 14 Apr 2009 17:15:49 UTC
Message-ID: <fa.8gwz/fAHaUtpJLBpHnp1/fcwhCI@ifi.uio.no>

On Tue, 14 Apr 2009, Alexey Dobriyan wrote:
> >
> > We're not merging OpenVZ code _either_.
>
> This is to give example of other prefixes: cpt_ and rst_
> Are they fine?

Do you secretly work for IBM?

IBM has a well-known disdain for vowels, and basically refuses to use them
for mnemonics (they were called on this, and did "eieio" as an instruction
just to try to make up for it).

But I'm from Finland. In Finnish, about 75% of all letters are vowels. I
find this dis-emvoweling to be stupid and impractical. Without vowels, you
can't tell Finnish words apart (admittedly, _with_ vowels, you generally
cannot pronounce them, so to a non-Finn it doesn't much matter).

My point is, let's go for a happy medium - LEAVE THE F*CKING VOWELS IN
PLACE ALREADY.

So let's call it "checkpoint" and "restore". Ok?

		Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [GIT PULL] percpu fixes for 2.6.32-rc6
Date: Thu, 12 Nov 2009 17:19:30 UTC
Message-ID: <fa.BiOpsW7A6sZVEVy92Tg6RTPg0rA@ifi.uio.no>

On Thu, 12 Nov 2009, Andres Baldrich wrote:
>
> (Kernel)/Documentation/CodingStyle
> line 83:

A lot of people have added code to CodingStyle. That doesn't make it
final. For example, that 80-column thing never existed in my original
coding style, for a reason.

I'm really inclined to just remove the stupid thing entirely both from
coding-style and from checkpatch.

80 columns do not matter. What matters is:
 - indentation
 - complex expressions and statements

and those two issues _together_ means that 80+ columns should be damn
rare, but the 80 columns itself is not at all that important.

Much more important than 80 columns should be the general guideline that a
"terminal window" may be as small as 80x24. But notice how 80 is just a
small part of that limitation - the 24 is as important as the 80. We have
a guideline that functions should fit on a screenful or two, ie we should
generally aim for functions to be <50 lines long.

And the 80-column thing is EXACTLY THE SAME THING. We should remember that
people may read the code using a roughly 80x24 screen size, but the same
way that nobody sane thinks that "24" is some hard limit on number of
lines, why do people suddenly think that "80" is a hard limit on the
number of columns?

		Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [PATCH] scripts/checkpatch.pl: Change long line warning to 105
Date: Fri, 18 Dec 2009 17:45:01 UTC
Message-ID: <fa.7mJMJiA5VBfPkCA3zgLLqOWKkUM@ifi.uio.no>

On Fri, 18 Dec 2009, Paul Mundt wrote:

> On Thu, Dec 17, 2009 at 09:12:24PM -0800, Joe Perches wrote:
> > On Thu, 2009-12-17 at 23:29 -0500, Valdis.Kletnieks@vt.edu wrote:
> > > Yeah, but I respectfully submit that if the regexp '^\t{6}' matches a non-
> > > continuation line, it's probably in its rights to whinge.
> > > 	grep -r -P '^\t{7:}(?!(.*,$|.*\);$))' . | more
> > > produces a whole lot of "this can't end well" output.
> >
> > I think this is a good test to add to checkpatch.
> >
> > Add 105 character long line WARN test
> > Add 80 character long line STRICT test
> > Add 6+ leading indent tabs test, consider restructuring
> >
> This looks like a reasonable compromise.

I like this. Except I think the indent test should count spaces too some
way. Or do we already warn about excessive space that should be tabs?

		Linus

From: tytso@mit.edu
Newsgroups: fa.linux.kernel
Subject: Re: [tip:sched/urgent] sched: Restore printk sanity
Date: Sun, 20 Dec 2009 22:13:36 UTC
Message-ID: <fa.1RJdfs/5afMiS7MYeZLXJZUsHCw@ifi.uio.no>

On Sun, Dec 20, 2009 at 08:15:31PM +0100, Ingo Molnar wrote:
>
> It's also needlessly broken mid-string. Checkpatch should warn about printk
> lines that end with a '"', those are almost always a sign of some ill-advised
> break-the-string artifact.

Ironic, given that for a long time checkpatch strongly encouraged
people to do this with its "line too long" complaint.  Maybe all
printk's should be exempted from this, or at least if it's the format
string which is breaking whatever the magic boundary happens to be?

Even if we up the limit to 106, or 132, or whatever, I can imagine
situations where it will be *impossible* to shut up checkpatch.
Either it will bitch and moan about the line being too long, or it
will bitch and moan that a format string has been broken across
multiple lines.  :-(

					- Ted