public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* Re: kernel guide to space
@ 2005-07-14  1:12 linux
  2005-07-20  3:41 ` Kyle Moffett
  0 siblings, 1 reply; 39+ messages in thread
From: linux @ 2005-07-14  1:12 UTC (permalink / raw)
  To: linux-kernel

>> I don't think there's a strict 80 column rule anymore.  It's 2005...

> Think again.  There are a lot of people who use 80 column windows so
> that we can see two code windows side-by-side.

Agreed.  If you're having trouble with width, it's a sign that the code
needs to be refactored.

Also, my personal rule is if that a source file exceeds 1000 lines, start
looking for a way to split it.  It can go longer (indeed, there is little
reason to split the fs/nls/nls_cp9??.c files), but 
(I will refrain from discussing drivers/scsi/advansys.c)



Comments on the rest of the thread:

> 3a. Binary operators
>	+ - / * %
>	== !=  > < >= <= && || 
>	& | ^ << >> 
>	= *= /= %= += -= <<= >>= &= ^= |=
>
>	spaces around the operator
>	a + b

Generally, yes, and if you violate this, take the spaces out around the
tightest-binding operators first!


>> I like this style because I can grep for ^function_style_for_easy_grep
>> and quickly find function def.

> That's a pretty bad argument, since most functions aren't declared
> that way, and there are much better source code navigational tools,
> like cscope and ctags.

Well, I use that style for everything I write, for exactly that reason,
so it's fairly popular.  Yes, there are lots of tools, but it's convenient
not to need them.

Also, definition argument lists can be a little longer than declaration
argument lists (due to the presence of argument names and possible
const qualifiers), so an extra place to break the line helps.

And it provides a place to put the handy GCC __attribute__(()) extensions...

static unsigned __attribute__((nonnull, pure))
is_condition_true(struct hairy *p, unsigned priority)
{
...
}

Finally, if you are burdened with long argument names, a shorter fixed prefix
makes it easier to align the arguments.  To pick a real-world example:

static sctp_disposition_t sctp_sf_do_5_2_6_stale(const struct sctp_endpoint *ep,
                                                 const struct sctp_association *
asoc,
                                                 const sctp_subtype_t type,
                                                 void *arg,
                                                 sctp_cmd_seq_t *commands)
I prefer to write

static sctp_disposition_t
sctp_sf_do_5_2_6_stale(const struct sctp_endpoint *ep,
                       const struct sctp_association *asoc,
                       const sctp_subtype_t type,
                       void *arg,
                       sctp_cmd_seq_t *commands)
Although in extreme cases, it's usually best to just to:
static sctp_disposition_t
sctp_sf_do_5_2_6_stale_bug_workaround(
	const struct sctp_endpoint *ep,
	const struct sctp_association *asoc,
	const sctp_subtype_t type,
	void *arg,
	sctp_cmd_seq_t *commands)


>> 3e. sizeof
>> 	space after the operator
>> 	sizeof a

> I use sizeof(a) always (both for sizeof(type) and sizeof(expr)).

You can, but I prefer not to.  Still, it behaves a lot "like a function",
so it's not too wrong.  In fact, I'll usually avoid the sizeof(type)
version entirely.  It's often clearer to replace, e.g.
	char buffer[sizeof(struct sctp_errhdr)+sizeof(union sctp_addr_param)];
with
	char buffer[sizeof *errhdr + sizeof *addrparm];
which (if you look at the code in sctp_sf_send_restart_abort), actually
reflects what's going on better.


What really gets my goat is
	return(0);

return *is not a function*.  Stop making it look syntactically like one!
That should be written
	return 0;

>> 3i. if/else/do/while/for/switch
>> 	space between if/else/do/while and following/preceeding
>> 	statements/expressions, if any:
>> 
>> 	if (a) {
>> 	} else {
>> 	}
>> 
>> 	do {
>> 	} while (b);

> What's wrong with if(expr) ? Rationale?

- It's less visually distinct from a function call.
- The space makes the keyword (important things, keywords) stand out more
  and makes it easier to pick out of a mass of code.
- (Subjective) it balances the space in the trailing ") {" better.

This matches my personal style.

>> 6. One-line statement does not need a {} block, so dont put it into one
>> 	if (foo)
>> 		bar;

> Disagree. Common case of hard-to-notice bug:
>
>	if(foo)
>		bar()
>...after some time code evolves into:
>	if(foo)
>		/*
>		 * We need to barify it, or else pagecache gets FUBAR'ed
>		 */
>		bar();

The braces should have been added then.  They are okay to omit when the
body contains one physical line of text, but my rule is that a comment or
broken expression requires braces:

	if (foo) {
		/* We need to barify it, or else pagecache gets FUBAR'ed */
		bar();
	}

	if (foo) {
		bar(p->foo[hash(garply) % LARGEPRIME]->head,
		    flags & ~(FLAG_FOO | FLAG_BAR | FLAG_BAZ | FLAG_QUUX));
	}

> Thus we may be better to slighty encourage use of {}s even if they are
> not needed:
>
>	if(foo) {
>		bar();
>	}

It's not horrible to include them, but it reduces clutter sometimes to
leave them out.


>>	if (foobar(.................................) + barbar * foobar(bar +
>>	                                                                foo *
>>	                                                                oof)) {
>>	}
> 
> Ugh, that's as ugly as it can get... Something like below is much
> easier to read...
> 
>	if (foobar(.................................) +
>	    barbar * foobar(bar + foo * oof)) {
>	}

Strongly agreed!  If you have to break an expression, do it at the lowest
precedence point possible!
 
> Even easier is
>	if (foobar(.................................)
>	    + barbar * foobar(bar + foo * oof)) {
>	}
>
> Since a statement cannot start with binary operators
> and as such we are SURE that there must have been something before.

I don't tend to do this, but I see the merit.  However, C uses a number
of operators (+ - * &) in both unary and binary forms, so it's
not always unambiguous.

In such cases, I'll usually move the brace onto its own line to make the
end of the condition clearer:

	if (foobar(.................................) +
	    barbar * foobar(bar + foo * oof))
	{
	}

Of course, better yet is to use a temporary or something to shrink
the condition down to a sane size, but sometimes you just need

	if (messy_condition_one &&
	    messy_condition_two &&
	    messy_condition_three)
	{
	}

^ permalink raw reply	[flat|nested] 39+ messages in thread
[parent not found: <4q0yr-4YQ-3@gated-at.bofh.it>]
[parent not found: <4p851-3Tl-11@gated-at.bofh.it>]
* kernel guide to space
@ 2005-07-11 14:56 Michael S. Tsirkin
  2005-07-11 15:34 ` Sander
                   ` (4 more replies)
  0 siblings, 5 replies; 39+ messages in thread
From: Michael S. Tsirkin @ 2005-07-11 14:56 UTC (permalink / raw)
  To: linux-kernel

Hi!
I've been tasked with edicating some new hires on linux kernel coding style.
While we have Documentation/CodingStyle, it skips detail that is supposed to
be learned by example.

Since I've been burned by this a couple of times myself till I learned,
I've put together a short list of rules complementing Documentation/CodingStyle.
This list is attached, below.

Please cc me directly with comments, if any.

Thanks,
MST

---

kernel guide to space AKA a boring list of rules
http://www.mellanox.com/mst/boring.txt

This text deals mostly with whitespace issues, hence the name.

Whitespace -- In computer science, a whitespace (or a whitespace character) is
any character which does not display itself but does take up space.
	From Wikipedia, the free encyclopedia.

1. Read Documentation/CodingStyle. Yes, it applies to you.
   When working on a specific driver/subsystem, try to follow
   the style of the surrounding codebase.

2. The last character on a line is never a whitespace
		Get a decent editor and don't leave whitespace at the end of
		lines.
			Documentation/CodingStyle

Whitespace issues:

3. Space rules for C

3a. Binary operators
	+ - / * %
	== !=  > < >= <= && || 
	& | ^ << >> 
	= *= /= %= += -= <<= >>= &= ^= |=

	spaces around the operator
	a + b

3b. Unary operators
	! ~
	+ - *
	&

	no space between operator and operand
	*a

3c. * in types
	Leave space between name and * in types.
	Multiple * dont need additional space between them.

	struct foo **bar;

3d. Conditional
	?:
	spaces around both ? and :
	a ? b : c

3e. sizeof
	space after the operator
	sizeof a

3f. Braces etc
	() [] -> .

	no space around any of these (but see 3h)
	foo(bar)

3g. Comma
	,
	space after comma, no space before comma
	foo, bar

3h. Semicolon
	;
	no space before semicolon
	foo;

3i. if/else/do/while/for/switch
	space between if/else/do/while and following/preceeding
	statements/expressions, if any:

	if (a) {
	} else {
	}

	do {
	} while (b);

3j. Labels
	goto and case labels should have a line of their own (possibly
	with a comment).
	No space before colon in labels.

int foobar()
{
	...
foolabel: /* short comment */
	foo();
}

4. Indentation rules for C
	Use tabs, not spaces, for indentation. Tabs should be 8 characters wide.

4a. Labels
	case labels should be indented same as the switch statement.
	statements occurring after a case label are indented by one level.

	switch (foo) {
	case foo:
		bar();
	default:
		break;
	}

4b. Global scope
	Functions, type definitions/declarations, defines, global
	variables etc are global scope. Start them at the first
	character in a line (indent level 0).

static struct foo *foo_bar(struct foo *first, struct bar *second,
			   struct foobar* thirsd);

4c. Breaking long lines
		Descendants are always substantially shorter than the parent
		and are placed substantially to the right.
			Documentation/CodingStyle

	Descendant must be indented at least to the level of the innermost
	compound expression in the parent. All descendants at the same level
	are indented the same.
	if (foobar(.................................) + barbar * foobar(bar +
				     					foo *
									oof)) {
	}

5. Blank lines
	One blank line between functions.

void foo()
{
}

/* comment */
void bar()
{
}

	No more than one blank line in a row.
	Last (or first) line in a file is never blank.

Non-whitespace issues:

6. One-line statement does not need a {} block, so dont put it into one
	if (foo)
		bar;

7. Comments
	Dont use C99 // comments.

8. Return codes
	Functions that return success/failure status, should use 0 for success,
	a negative value for failure.
	Error codes are in linux/errno.h .

	if (do_something()) {
		handle_error();
		return -EINVAL;
	}

	Functions that test a condition return 1 if condition is satisfied,
	0 if its not.

	if (is_condition())
		condition_true();

9. Data types
	Standard linux types are in linux/types.h .
	See also Linux Device Drivers, Third Edition,
	Chapter 11: Data Types in the Kernel.  http://lwn.net/images/pdf/LDD3/

9a. Integer types
	int is the default integer type.
	Use unsigned type if you perform bit operations (<<,>>,&,|,~).
	Use unsigned long if you have to fit a pointer into integer.
	long long is at least 64 bit wide on all platforms.
	char is for ASCII characters and strings.
	Use u8,u16,u32,u64 if you need an integer of a specific size.

9b. typedef
	Using typedefs to hide the data type is generally discouraged.
	typedefs to function types are ok, since these can get very long.

typedef struct foo *(foo_bar_handler)(struct foo *first, struct bar *second,
				      struct foobar* thirsd);

Editor configuration:

9. The following is helpful with VIM
	set cinoptions=(0:0

Michael S. Tsirkin

-- 
MST

^ permalink raw reply	[flat|nested] 39+ messages in thread
end of thread, other threads:[~2005-07-23  1:30 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-07-14  1:12 kernel guide to space linux
2005-07-20  3:41 ` Kyle Moffett
2005-07-20  7:52   ` Jan Engelhardt
2005-07-21  0:45     ` Paul Jackson
2005-07-21  6:22       ` Jan Engelhardt
2005-07-21 16:57       ` Kyle Moffett
2005-07-21 18:42         ` Jesper Juhl
2005-07-21 19:37           ` linux-os (Dick Johnson)
2005-07-21 20:11             ` Jesper Juhl
2005-07-22  1:32               ` Jesper Juhl
2005-07-23  1:30                 ` Jesper Juhl
2005-07-22  2:29             ` Miles Bader
2005-07-22  3:47               ` Paul Jackson
     [not found] <4q0yr-4YQ-3@gated-at.bofh.it>
     [not found] ` <4sdKS-7Ko-9@gated-at.bofh.it>
     [not found]   ` <4shEU-25p-5@gated-at.bofh.it>
2005-07-20 17:42     ` Bodo Eggert
     [not found] <4p851-3Tl-11@gated-at.bofh.it>
     [not found] ` <4p8HK-4he-19@gated-at.bofh.it>
     [not found]   ` <4pmUD-7gx-37@gated-at.bofh.it>
2005-07-12 19:36     ` Bodo Eggert
2005-07-13  5:46       ` Denis Vlasenko
  -- strict thread matches above, loose matches on Subject: below --
2005-07-11 14:56 Michael S. Tsirkin
2005-07-11 15:34 ` Sander
2005-07-12  6:52   ` Denis Vlasenko
2005-07-12 11:55     ` Patrick McHardy
2005-07-12 12:17       ` Richard B. Johnson
2005-07-13  6:58         ` Paul Jackson
2005-07-13 17:22           ` Lee Revell
2005-07-13 17:52             ` Paul Jackson
2005-07-13 23:38             ` Marc Singer
2005-07-11 15:44 ` Dmitry Torokhov
2005-07-11 17:19   ` Ingo Oeser
2005-07-12  7:12 ` Denis Vlasenko
2005-07-12 11:36   ` Domen Puncer
2005-07-13  7:09 ` Paul Jackson
2005-07-20 12:59 ` Jesper Juhl
2005-07-20 13:07   ` Michael S. Tsirkin
2005-07-20 21:05   ` Paul Jackson
2005-07-20 22:37   ` Krzysztof Halasa
2005-07-22 17:12     ` Patrick Draper
2005-07-22 17:51       ` Jesper Juhl
2005-07-22 19:21       ` Sam Ravnborg
2005-07-22 20:28         ` Jesper Juhl
2005-07-21  0:20   ` Horst von Brand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox