All of this is of course up for debate, but here is an outline of what I feel is a reasonable code convention for C code. I've included arguments where appropriate. I think.
A tab is exactly four spaces. So no hard tabs (\t) anywhere (except in makefiles, where they are required).
Several levels of nesting is difficult to understand. If you find yourself in a for in an if in a while -- refactor! Break it up into smaller functions with descriptive names.
A line length of 78 characters is reasonable, and helps keep the code readable. (Why 78? Well, my editor is set to 78 by default ...) If you fall off the end of the line, it is probably a good indication that some refactoring should be done.
If you feel the need to write a comment to explain a piece of code -- think twice. Maybe you could break things up into smaller functions, with better names, etc.? See the previous section on Nesting.
Comments are nice, but only if they convey something that the code simply cannot.
Single-line comments (no slash-slash in C, that's C++!):
/* Pretty much like this. */
int x ...;
Multi-line comments:
/*
* The code below ... ZOMG!
* Watch out!
*/
for (...)
{
... really complicated code ... ouch ...
}
Code can often be divided into logical sections. Comments may be used to further emphasize this. Some examples:
/*
* ---------------------------------------------------------------------------
* Globals.
* ---------------------------------------------------------------------------
*/
/*
* All list nodes available in the system, organised as a freelist.
*/
static list_node_t
g_list_nodes[NUM_LIST_NODES];
...
/*
* ---------------------------------------------------------------------------
* Macros.
* ---------------------------------------------------------------------------
*/
/*
* Counts the number of elements in an array. Note that this only works with
* an actual array, and not a pointer which used to be an array (like an
* array passed to a function)!
*/
#define COUNT_ARRAY(arr) ( sizeof(arr) / sizeof((arr)[0]) )
...
/*
* ---------------------------------------------------------------------------
* Types.
* ---------------------------------------------------------------------------
*/
/*
* A generic list node. Could be used as pretty much any type of list node.
* Just add more pointers as needed.
*/
typedef struct list_node list_node_t;
struct list_node
{
...
};
...
/*
* ---------------------------------------------------------------------------
* Functions.
* ---------------------------------------------------------------------------
*/
/*
* Initializes the freelist. This function must be called before calling any
* other list node functions.
*/
void
init_list_node_freelist(void)
{
...
}
...
As seen in the previous section, documentation comments (preferably multi-line, to stand out) should be used to describe globals, macros, functions etc. I don't think we need to be overly strict with these, a couple of sentences on what the function accomplishes and possible pitfalls etc. is probably more than adequate.
Braces always line up in the same column. This makes it easier to quickly associate matching pairs:
struct pcb
{
uint32_t priority;
};
pcb_t *
pcb_alloc(void)
{
if (...)
{
...
}
}
Prefer brackets over no brackets. This makes it easier to maintain code. Do this:
if (...)
{
...
}
else
{
...
}
instead of:
if (...)
...
else
...
Since language constructs aren't function calls, they should look a little different (also read the section on Function Declarations), so keep a single space after the keyword, just before any parentheses:
if (...)
{
...
}
while (...)
{
...
}
for (i = 0; i < 10; ++i)
{
...
}
UPDATE: Forward references seem unnecessary. Lets skip these.
Declaring a struct involves two separate steps: a typedef and the struct definition itself. The typedef (which should end in '_t') lets us write more concise code. By starting with the typedef, we allow the struct definition to use the alias:
typedef struct pcb pcb_t;
struct pcb
{
pcb_t *next_free;
};
Both of these parts must be part of the header file if and only if the struct must be part of the interface. Private (local to the .c file) structs should of course only be visible within the .c file.
Just like with global variable declarations, keep a newline just before the function name, and avoid spaces between the name and the argument list:
pcb_t *
pcb_alloc(void);
void
pcb_free(pcb_t *pcb);
void
pcb_init_freelist(void);
As you can see, it's easy to quickly scan the function names, since they are all collected in the leftmost column.
If a function takes no arguments, then it should be declared (and defined) as taking void. A function that is typed to nothing, not even void, is assumed to take a variable number of arguments.
When a function is meant to accept an array, prototype and define it as an actual array (with unspecified size), and not just a pointer (even though they are interchangeable in this situation):
void
print_str(const char str[]);
void
print_str(const char str[])
{
puts(str);
}
Also, as seen above, when a function can guarantee that it does not change any data being pointer to by argument pointers, qualify those arguments with 'const' (meaning read-only). This is both self-documenting and might enable the compiler to optimize more aggressively (not that we have any optimizations turned on, but hey ...).
Use it only for very concise expressions. Never nest. This might be okay, but would probably look better as a regular if:
int
fib(int n)
{
return (i <= 1) ? 0 : fib(n-1)+fib(n-2);
}
As for pointers, always place the asterisk (*) to the right (if there is choice in the matter):
pcb_t *pcb = ...;
pcb_t *
pcb_alloc(void);
Why, you say? Isn't the pointer part of the type? No, not really. Consider this (which is awful, and should be avoided, no less because of this reason):
int* p1, p2;
Did you expect both p1 and p2 to be pointers to int? Too bad. Only p1 is a pointer. With better formatting, the code looks this:
int *p1, p2;
It's now much more obvious that only p1 is a pointer. So how do we turn p2 into a pointer then? The best way is of course to break it out, which in the long run also makes maintenance easier:
int *p1;
int *p2;
But that's not the answer you were looking for, now is it. This is how you do it the obscure way:
int *p1, *p2;
Always initialize local variables (static variables are always initialized to zero). Using an uninitialized variable can lead to missile launches and system crashes.
Integral types are usually initialized to 0, and pointers to NULL. Structs and arrays can be neatly zeroed using the macros ZERO_STRUCT and ZERO_ARRAY found in utils.h:
pcb_t pcb;
char str[20];
ZERO_STRUCT(&pcb);
ZERO_ARRAY(str);
Variable declarations must appear first in a block of braces (this is actually required by ANSI C). Also try to keep variables as local as possible to their usage:
if (...)
{
size_t i = 0;
for (i = 0; ...)
{
char c = getchar();
putchar(c);
}
}
If you declare variables on the top-level (outside of any functions), they are accessible throughout the entire file. If they are not mentioned in a header file as part of an interface, they should be marked as static, to make them private to the file they were declared in:
static pcb_t *
g_freelist;
Any variable that is not local to a function (i.e. declared on the top-level) should be prefixed with the two characters 'g_'. This makes it easy to spot their usage inside functions.
Since global variables tend to have more or less long storage qualifiers (static, volatile, const, etc.) it might be nice to have a newline just before the variable name. This neatly lines up all symbol names in the leftmost column, which makes them easier to scan:
static pcb_t *
g_freelist;
static volatile tty_t *const
tty = (tty_t *)0x...;
Use lowercase letters only, and underscores for spaces. As per the previous section on Global Variables, prefix globals with 'g_'. [Should we perhaps use some convention for pointers too? Such as 'pcb_t **pp_var' meaning pp_var is a 'pointer to pointer to pcb_t'.]
Prefer long names over short ones, and avoid non-customary abbreviations. A simple rule of thumb is "the smaller the scope, the shorter the name". Which roughly means that globals must have descriptive and possibly long names (g_pcb_freelist), while short-lived variables, such as loop counters, should be kept as short and idiomatic as possible (i, j, next).
Since constants are pretty much global, always prefer longer names. By convention, always use UPPERCASE letters and underscores for spaces:
#ifndef KERNEL_STACK_SIZE
#define KERNEL_STACK_SIZE 0x4000
#endif
As you can see, also wrap the #define in an #ifndef which uses the very same constant. This enables us to set various settings at compile time, for example in the Makefile, instead of searching around in all the header files. [Maybe we should keep a list somewhere of all constants that describe system "settings"?]
If the replacement isn't atomic, wrap it in parentheses (and read the next section):
#define BIT3 (1 << 2)
This avoids unnecessary headaches involving operator precedence. Note that this kind of macro isn't a configurable "setting", so no #ifndef here.
Avoid! If you must use them, always wrap the entire expression and all arguments in parentheses:
#define MAX(a,b) ( ((a) > (b)) ? (a) : (b) )
If you don't know why you should do this, then avoid macros altogether.
Always use "include guards" to avoid wasteful (and most likely erroneous) inclusion. For example, in file 'file_1.h', wrap the entire code in:
#ifndef FILE_1_H
#define FILE_1_H
... file contents here ...
#endif