WARNING: This is the _old_ Lustre wiki, and it is in the process of being retired. The information found here is all likely to be out of date. Please search the new wiki for more up to date information.

Difference between revisions of "Coding Guidelines"

From Obsolete Lustre Wiki
Jump to navigationJump to search
(Lustre coding guidelines)
Line 1: Line 1:
 
'''''Note:''''' ''All Lustre developers should follow the guidelines in this page very strictly to avoid problems during code merges later on. Please make the required changes to the default formatting rules in the editor you use to comply to the guidelines below.
 
 
 
== Beautiful Code ==
 
== Beautiful Code ==
  
''A note from Eric Barton, our lead Lustre developer:''
+
''A note from Eric Barton, our lead engineer:''
 +
           
 +
More important than the physical layout of code (which is covered in detail below) is the idea that the code should be ''beautiful'' to read.
  
More important than the physical layout of code (which is covered in detail below) is the thought that code is ''beautiful'' to read.  What makes code "beautiful" to me?  Fundamentally, I'm talking about readability and obviousness - the code must not have secrets.
+
What makes code beautiful to me?  Fundamentally, it's readability and obviousness.  The code must not have secrets but should flow easily, pleasurably and ''accurately'' off the page and into the mind of the reader.
  
* Separate "ideas" are clearly separated in the program layout -  e.g. blank lines group related statements and separate unrelated statements.
+
How do I think beautiful code is written?  Like this...
  
* Declarations are easy to read.  The declarations should always be separated from the functional codeDeclarations should be made as locally as possible to the code they are being used.  I prefer one variable per line, with the names aligned vertically - it makes checking types as you read quicker and IMHO it's a pleasure to the eye.
+
* The author must be confident and knowledgeable and proud of her workShe must understand what the code should do, the environment it must work in, all the combinations of inputs, all the valid outputs, all the possible races and all the reachable states.  She must  [http://en.wikipedia.org/wiki/Grok grok] it.
  
* If it's a choice between breaking the 80-chars-per-line rule and code that doesn't look good (e.g. breaks indentation rules or doesn't visually contain procedure call parameters between the brackets or spreads over too many lines), then break the 80-chars-per-line rule.  We _don't_ want stupidly long lines, but we all have enough screen resolution these days to not have to wrap lines at 80 chars.
+
* Names must be well chosen.  The meaning a human reader attaches to a name can be orthogonal to what the compiler does with it so it's just as easy to mislead as it is to inform.  ''[http://en.wikipedia.org/wiki/Does_what_it_says_on_the_tin "Does exactly what it says on the tin"]'' is a popular UK English expression describing something that does ''exactly'' what it tells you it's going to do, no more and no less.  For example, if I open a tin labelled "soap", I expect the contents to help me wash and maybe even smell nice.  If it's no good at removing dirt I'll be disappointed. If it removes the dirt but burns off a layer of skin with it, I'll be positively upset. The name of a procedure, a variable or a structure member should tell you something informative about the entity without misleading - just "what it says on the tin".
  
* Names are well chosen.  ''"Does exactly what it says on the tin"'' is an English (UK) expression describing something that tells you what it's going to do and then does _exactly_ thatFor example, when I open a box labelled "soap", I expect it to help me wash and maybe even smell niceI'll get upset if it's no good at removing dirt, and really upset if it makes me break out in a rashThe name of a procedure or variable or structure member should tell you something about the entity, without giving you misleading information - just "what it says on the tin".
+
* Names must be well chosen.  Local, temporary variables can almost always remain relatively short and anonymous, while names in global scope must be uniqueIn general the wider the context you expect to use the name in, the more unique and informative the name should beDon't be scared of long names if they help to '''make_the_code_clearer''', but '''do_not_let_things_get_out_of_hand''' either - we don't write COBOL.  Related names should be obvious, unambiguous and avoid naming conflicts with other unrelated names, e.g. by using a consistent prefix.  This applies to all API procedures (if not all procedures period) withing a given subsystemSimilarly unique member names for global structures, using a prefix to identify the parent structure type helps readability.
  
* Names are well chosen.  This time I mean that you don't choose names that easily become a different but valid (to the compiler) name if you make a spelling mistake.  '''i''' and '''j''' aren't the worst example - '''req_portal''' and '''rep_portal''' are much worse (and taken from our own code!!!).
+
* Names must be well chosen.  Don't choose names that are easily confused - especially not if the compiler can't even tell the difference when you make a spelling mistake.  '''i''' and '''j''' aren't the worst example - '''req_portal''' and '''rep_portal''' are much worse (and taken from our own code!!!).
  
* Names are well chosen.  Don't be scared of long names, but_do_not_go_to_extremes_either - we don't write COBOL.
+
* Names must be well chosen.  I can't emphasise this issue enough - I hope you get the point.
  
* Names are well chosen.  I can't emphasise this issue enough - I hope you get the point.
+
* Assertions must be used intelligently.  They can be abused - obviously when over-used they hurt performance and can even convey the impression the author didn't know what she was doing and added them to help her ''learn'' the code.  But when assertions are chosen carefully and used properly, they combine the roles of ''active comment'' and ''software fuse''As an ''active comment'' they tell you something about the program which you can trust more than a comment.  And as a ''software fuse'', they provide fault isolation between subsystems by letting you know when and where invariant assumptions are violated.
  
* Intelligent use of assertionsAssertions can be abusedObviously, when over-used they hurt performance. And they can also make you think that the code author didn't know what she was doing and added them to help her ''learn'' the code.  But when assertions are used properly, they combine the roles of ''active comment'' and ''software fuse''.  As an ''active comment'' they tell you something about the program which you can trust more than a commentAs a ''software fuse'', they provide fault isolation between subsystem by letting you know when and where invariant assumptions are violated.
+
* Formatting and indentation rules should be followed intelligently.  The visual layout of the code on the page should lend itself to being read easily and accurately - it just looks clean and good.
 +
** Separate "ideas" should be separated clearly in the code layout using blank lines that group related statements and separate unrelated statements.
 +
** Procedures should not ramble onYou must be able to take in the meaning of a procedure without scrolling past page after page of code or parsing deeply nested conditionals and loopsThe 80-character rule is there for a reason.
 +
** Declarations are easier to refer to while scanning the code if placed in a block locally to, but visually separate from the code that uses themReadability is further enhanced by limiting declarations to one per line and aligning types and names vertically.
 +
** Parameters in multiline procedure calls should be aligned so that they are visually contained by their brackets.
 +
** Brackets should be used in expressions to make operator precedence clear.
 +
** Conditional boolean ('''if (expr)'''), scalar ('''if (val != 0)''') and pointer ('''if (ptr != NULL)''') expressions should be written consistently.
 +
** Formatting and indentation rules should not be followed slavishlyIf you're faced with either breaking the 80-chars-per-line rule or the parameter indentation rule or creating an obscure helper function, then the 80-chars-per-line rule might have to suffer. The overriding consideration is how the code reads.
  
I could go on, but I hope you get the idea.  Notice that I didn't mention ''clever'' as a desirable attribute.  It's only one step from
+
I could go on, but I hope you get the idea.  Just think about the poor reader when you're writing, and whether your code will convey its meaning naturally, quickly and accurately, without room for misinterpretation.  I didn't mention ''clever'' as a feature of beautiful code because it's only one step from ''clever'' to ''tricky'' - consider...
''clever'' to ''tricky'' - consider...
 
  
 
   t = a; a = b; b = t; /* dumb swap */
 
   t = a; a = b; b = t; /* dumb swap */
Line 31: Line 36:
 
   a ^= b; b ^= a; a ^= b;  /* clever swap */
 
   a ^= b; b ^= a; a ^= b;  /* clever swap */
  
...which is a very minor example.  It can almost be excused, because the "cleverness" is confined to a tiny part of the code.  But if the ''clever'' code had sub-structure, that sub-structure would be very hard to work on.  You can't be sure you're not screwing something up until you understand completely the environment in which you're working.
+
You could feel quite pleased that the clever swap avoids the need for a local temporary variable - but is that such a big deal compared with how quickly, easily and accurately the reader will read it?  This is a very minor example which can almost be excused because the "cleverness" is confined to a tiny part of the code.  But when ''clever'' code gets spread out it becomes much harder to modify without adding defects.  You can only work on code without screwing up if you understand the code ''and'' the environment it works in completely.  Or to put it more succinctly...
  
 
''Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.'' - [http://en.wikipedia.org/wiki/Brian_Kernighan  Brian W. Kernighan]
 
''Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.'' - [http://en.wikipedia.org/wiki/Brian_Kernighan  Brian W. Kernighan]
  
IMHO, beautiful code helps code quality because it improves communication between the code author and the code reader.  Since code modifiers are also code readers, the quality of communication can lead either to a virtuous circle of improving quality, or a vicious circle of degrading quality.
+
IMHO, beautiful code helps code quality because it improves communication between the code author and the code reader.  Since everyone maintaining and developing the code is a code reader as well as a code author, the quality of this communication can lead either to a virtuous circle of improving quality, or a vicious circle of degrading quality.  You, dear reader, will determine which.
 +
 
 +
== Coding Style ==
 +
 
 +
All of our formatting, wrapping, parenthesis, brace placement, etc. rules are originally derived from the [http://www.kernel.org/doc/Documentation/CodingStyle Linux kernel rules], which are basically K&R.
  
== Formatting Guidelines ==
+
=== Formatting Guidelines ===
  
Here are some guidelines to formatting Lustre code.
+
'''Note:''' ''All Lustre developers and contributors should follow these guidelines strictly to avoid spurious whitespace differences which create problems later for code merges.  Please ensure the default formatting rules in whichever editor you use comply with them.''
  
* There should be no tabs in any Lustre or LNET files. The exceptions are libsysio (maintained by someone else), ldiskfs and kernel patches (also part of a non-Lustre Group project).
+
* There should be no tabs in any Lustre, LNET or libcfs files. The exceptions are libsysio (maintained by someone else), ldiskfs and kernel patches (also part of a non-Lustre Group project).
  
 
* Blocks should be indented 8 spaces.
 
* Blocks should be indented 8 spaces.
 
* Variables should be declared one per line, even if there are multiple variables of the same type.
 
  
 
* New files should contain the following along with the license boilerplate.  This will cause vim and emacs to use spaces instead of tabs for indenting.  If you use a different editor, it also needs to be set to use spaces for indening Lustre code.
 
* New files should contain the following along with the license boilerplate.  This will cause vim and emacs to use spaces instead of tabs for indenting.  If you use a different editor, it also needs to be set to use spaces for indening Lustre code.
Line 54: Line 61:
 
</pre>
 
</pre>
  
* All lines should wrap at 80 characters. If it's getting too hard to wrap there, you probably need to break it up into more functions.  In some cases, it is acceptable to remove a few spaces between function arguments to avoid overflowing onto the next line.
+
* All lines should wrap at 80 characters. If it's getting too hard to wrap there, you probably need to break it up into more functions.
  
* Do not have spaces or tabs on blank lines or at the end of lines.  Find these with some regexps in your patch (grep, or in vim) before attaching it to bugzilla:
+
* Do not have spaces or tabs on blank lines or at the end of lines.  Please ensure you remove all instances of these in any patches you submit to bugzilla.  You can find them with grep or in vim using the following regexps...
 
<pre style="background:lightgrey;">
 
<pre style="background:lightgrey;">
 
  /[ \t]$/
 
  /[ \t]$/
Line 72: Line 79:
  
 
:In emacs, you can use (whitespace-mode) or (whitespace-visual-mode) depending on version. You should also consider using (flyspell-prog-mode).
 
:In emacs, you can use (whitespace-mode) or (whitespace-visual-mode) depending on version. You should also consider using (flyspell-prog-mode).
 +
 +
=== Language Features ===
  
 
* Don't use "inline" unless you're doing something so performance critical that the function call overhead will make a difference -- in other words: almost never.  It makes debugging harder.
 
* Don't use "inline" unless you're doing something so performance critical that the function call overhead will make a difference -- in other words: almost never.  It makes debugging harder.
  
'''''Note:''''' All of our wrapping, parenthesis, brace placement, etc. rules are basically Linux kernel rules, which are basically K&R. For those of you in need of a refresher, [[#More Details]] are provided below.
+
* Use ''typedef'' carefully...
 +
** Do not create a new integer typedef without a really good reason.
 +
** Always postfix typedef names with '''_t''' so that they can be identified clearly in the code.
 +
** Never typedef pointers. The '''*''' makes C pointer declarations obvious - hiding it obfuscates the code.
  
* For Autoconf macros, follow the [http://www.gnu.org/software/autoconf/manual/html_node/Coding-Style.html style suggested in the autoconf manual].
+
* Variable should be declared one per line, type and name, even if there are multiple variables of the same type.  For maximum readability, the names should be aligned on the same column.
 
+
<pre style="background:lightgrey;">
<pre style="background:lightgrey;">
+
  right:
    AC_CACHE_CHECK([for EMX OS/2 environment], [ac_cv_emxos2],
 
    [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [return __EMX__;])],
 
                        [ac_cv_emxos2=yes],
 
                        [ac_cv_emxos2=no])])
 
</pre>
 
:or even
 
<pre style="background:lightgrey;">
 
    AC_CACHE_CHECK([for EMX OS/2 environment],
 
                    [ac_cv_emxos2],
 
                    [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],
 
                                                        [return __EMX__;])],
 
                                      [ac_cv_emxos2=yes],
 
                                      [ac_cv_emxos2=no])])
 
</pre>
 
 
 
==== More Details ====
 
  
* Each variable declaration should be on a separate line, with the names aligned to the same column:
 
<pre style="background:lightgrey;">
 
 
   int          len;
 
   int          len;
 
   int          count;
 
   int          count;
 +
  struct inode *inode;
 +
</pre>
 +
<pre style="background:lightgrey;">
 +
  wrong:
 +
 +
  int len, count;
 
   struct inode *inode;
 
   struct inode *inode;
 
  </pre>
 
  </pre>
Line 118: Line 118:
 
                                                       foo_argument),
 
                                                       foo_argument),
 
                                       last_argument);
 
                                       last_argument);
+
 
 
   if (some_long_condition(arg1, arg2, arg3) < some_long_value &&
 
   if (some_long_condition(arg1, arg2, arg3) < some_long_value &&
 
       another_long_condition(very_long_argument_name,
 
       another_long_condition(very_long_argument_name,
Line 124: Line 124:
 
       second_long_value) {
 
       second_long_value) {
 
   }
 
   }
  </pre>                        
+
  </pre>
 
  <pre style="background:lightgrey;">
 
  <pre style="background:lightgrey;">
 
   wrong:
 
   wrong:
Line 131: Line 131:
 
                     longest_argument(sub_argument, foo_argument),
 
                     longest_argument(sub_argument, foo_argument),
 
                     last_argument);
 
                     last_argument);
+
 
 
   if (some_long_condition(arg1, arg2, arg3) < some_long_value &&
 
   if (some_long_condition(arg1, arg2, arg3) < some_long_value &&
 
               another_long_condition(very_long_argument_name,
 
               another_long_condition(very_long_argument_name,
Line 188: Line 188:
 
   }
 
   }
 
</pre>
 
</pre>
<pre style="background:lightgrey;">  
+
<pre style="background:lightgrey;">
 
   wrong:
 
   wrong:
+
 
 
   if (foo) {
 
   if (foo) {
 
           bar();
 
           bar();
Line 200: Line 200:
 
<pre style="background:lightgrey;">
 
<pre style="background:lightgrey;">
 
   right:
 
   right:
+
 
 
   #define DO_STUFF(a)                              \
 
   #define DO_STUFF(a)                              \
 
   do {                                            \
 
   do {                                            \
Line 209: Line 209:
 
<pre style="background:lightgrey;">
 
<pre style="background:lightgrey;">
 
   wrong:
 
   wrong:
+
 
 
   #define DO_STUFF(a) \
 
   #define DO_STUFF(a) \
 
   { \
 
   { \
Line 239: Line 239:
 
   * stuff */
 
   * stuff */
 
   # endif
 
   # endif
   #endif /* __KERNEL__ */  
+
   #endif /* __KERNEL__ */
 
</pre>
 
</pre>
 
* Comments should have the leading '''/*''' on the same line as the comment, and the trailing '''*/''' at the end of the last comment line.  Intermediate lines should start with a '''*''' aligned with the first line's '''*''':
 
* Comments should have the leading '''/*''' on the same line as the comment, and the trailing '''*/''' at the end of the last comment line.  Intermediate lines should start with a '''*''' aligned with the first line's '''*''':
<pre style="background:lightgrey;">  
+
<pre style="background:lightgrey;">
 
   /* This is a short comment */
 
   /* This is a short comment */
+
 
 
   /* This is a multi-line comment.  I wish the line would wrap already,
 
   /* This is a multi-line comment.  I wish the line would wrap already,
 
   * as I don't have much to write about. */
 
   * as I don't have much to write about. */
Line 289: Line 289:
 
   loff_t                %lld after a cast to long long (unfortunately)
 
   loff_t                %lld after a cast to long long (unfortunately)
 
</pre>
 
</pre>
 +
 +
* For Autoconf macros, follow the [http://www.gnu.org/software/autoconf/manual/html_node/Coding-Style.html style suggested in the autoconf manual].
 +
 +
<pre style="background:lightgrey;">
 +
    AC_CACHE_CHECK([for EMX OS/2 environment], [ac_cv_emxos2],
 +
    [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [return __EMX__;])],
 +
                        [ac_cv_emxos2=yes],
 +
                        [ac_cv_emxos2=no])])
 +
</pre>
 +
:or even
 +
<pre style="background:lightgrey;">
 +
    AC_CACHE_CHECK([for EMX OS/2 environment],
 +
                    [ac_cv_emxos2],
 +
                    [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],
 +
                                                        [return __EMX__;])],
 +
                                      [ac_cv_emxos2=yes],
 +
                                      [ac_cv_emxos2=no])])
 +
</pre>
 +
 
----
 
----
''
 

Revision as of 19:48, 2 September 2009

Beautiful Code

A note from Eric Barton, our lead engineer:

More important than the physical layout of code (which is covered in detail below) is the idea that the code should be beautiful to read.

What makes code beautiful to me? Fundamentally, it's readability and obviousness. The code must not have secrets but should flow easily, pleasurably and accurately off the page and into the mind of the reader.

How do I think beautiful code is written? Like this...

  • The author must be confident and knowledgeable and proud of her work. She must understand what the code should do, the environment it must work in, all the combinations of inputs, all the valid outputs, all the possible races and all the reachable states. She must grok it.
  • Names must be well chosen. The meaning a human reader attaches to a name can be orthogonal to what the compiler does with it so it's just as easy to mislead as it is to inform. "Does exactly what it says on the tin" is a popular UK English expression describing something that does exactly what it tells you it's going to do, no more and no less. For example, if I open a tin labelled "soap", I expect the contents to help me wash and maybe even smell nice. If it's no good at removing dirt I'll be disappointed. If it removes the dirt but burns off a layer of skin with it, I'll be positively upset. The name of a procedure, a variable or a structure member should tell you something informative about the entity without misleading - just "what it says on the tin".
  • Names must be well chosen. Local, temporary variables can almost always remain relatively short and anonymous, while names in global scope must be unique. In general the wider the context you expect to use the name in, the more unique and informative the name should be. Don't be scared of long names if they help to make_the_code_clearer, but do_not_let_things_get_out_of_hand either - we don't write COBOL. Related names should be obvious, unambiguous and avoid naming conflicts with other unrelated names, e.g. by using a consistent prefix. This applies to all API procedures (if not all procedures period) withing a given subsystem. Similarly unique member names for global structures, using a prefix to identify the parent structure type helps readability.
  • Names must be well chosen. Don't choose names that are easily confused - especially not if the compiler can't even tell the difference when you make a spelling mistake. i and j aren't the worst example - req_portal and rep_portal are much worse (and taken from our own code!!!).
  • Names must be well chosen. I can't emphasise this issue enough - I hope you get the point.
  • Assertions must be used intelligently. They can be abused - obviously when over-used they hurt performance and can even convey the impression the author didn't know what she was doing and added them to help her learn the code. But when assertions are chosen carefully and used properly, they combine the roles of active comment and software fuse. As an active comment they tell you something about the program which you can trust more than a comment. And as a software fuse, they provide fault isolation between subsystems by letting you know when and where invariant assumptions are violated.
  • Formatting and indentation rules should be followed intelligently. The visual layout of the code on the page should lend itself to being read easily and accurately - it just looks clean and good.
    • Separate "ideas" should be separated clearly in the code layout using blank lines that group related statements and separate unrelated statements.
    • Procedures should not ramble on. You must be able to take in the meaning of a procedure without scrolling past page after page of code or parsing deeply nested conditionals and loops. The 80-character rule is there for a reason.
    • Declarations are easier to refer to while scanning the code if placed in a block locally to, but visually separate from the code that uses them. Readability is further enhanced by limiting declarations to one per line and aligning types and names vertically.
    • Parameters in multiline procedure calls should be aligned so that they are visually contained by their brackets.
    • Brackets should be used in expressions to make operator precedence clear.
    • Conditional boolean (if (expr)), scalar (if (val != 0)) and pointer (if (ptr != NULL)) expressions should be written consistently.
    • Formatting and indentation rules should not be followed slavishly. If you're faced with either breaking the 80-chars-per-line rule or the parameter indentation rule or creating an obscure helper function, then the 80-chars-per-line rule might have to suffer. The overriding consideration is how the code reads.

I could go on, but I hope you get the idea. Just think about the poor reader when you're writing, and whether your code will convey its meaning naturally, quickly and accurately, without room for misinterpretation. I didn't mention clever as a feature of beautiful code because it's only one step from clever to tricky - consider...

  t = a; a = b; b = t; /* dumb swap */
  a ^= b; b ^= a; a ^= b;  /* clever swap */

You could feel quite pleased that the clever swap avoids the need for a local temporary variable - but is that such a big deal compared with how quickly, easily and accurately the reader will read it? This is a very minor example which can almost be excused because the "cleverness" is confined to a tiny part of the code. But when clever code gets spread out it becomes much harder to modify without adding defects. You can only work on code without screwing up if you understand the code and the environment it works in completely. Or to put it more succinctly...

Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it. - Brian W. Kernighan

IMHO, beautiful code helps code quality because it improves communication between the code author and the code reader. Since everyone maintaining and developing the code is a code reader as well as a code author, the quality of this communication can lead either to a virtuous circle of improving quality, or a vicious circle of degrading quality. You, dear reader, will determine which.

Coding Style

All of our formatting, wrapping, parenthesis, brace placement, etc. rules are originally derived from the Linux kernel rules, which are basically K&R.

Formatting Guidelines

Note: All Lustre developers and contributors should follow these guidelines strictly to avoid spurious whitespace differences which create problems later for code merges. Please ensure the default formatting rules in whichever editor you use comply with them.

  • There should be no tabs in any Lustre, LNET or libcfs files. The exceptions are libsysio (maintained by someone else), ldiskfs and kernel patches (also part of a non-Lustre Group project).
  • Blocks should be indented 8 spaces.
  • New files should contain the following along with the license boilerplate. This will cause vim and emacs to use spaces instead of tabs for indenting. If you use a different editor, it also needs to be set to use spaces for indening Lustre code.
  /* -*- mode: c; c-basic-offset: 8; indent-tabs-mode: nil; -*-
   * vim:expandtab:shiftwidth=8:tabstop=8:
   */
  • All lines should wrap at 80 characters. If it's getting too hard to wrap there, you probably need to break it up into more functions.
  • Do not have spaces or tabs on blank lines or at the end of lines. Please ensure you remove all instances of these in any patches you submit to bugzilla. You can find them with grep or in vim using the following regexps...
 /[ \t]$/
Alternatively, if you use vim you can put this line in your vimrc file, which will highlight whitespace at the end of lines and spaces followed by tabs in indentation (only works for C/C++ files):
 let c_space_errors=1
Or you can use this command, which will make tabs and whitespace at the end of lines visible for all files (but a bit more discretely):
 set list listchars=tab:>\ ,trail:$
In emacs, you can use (whitespace-mode) or (whitespace-visual-mode) depending on version. You should also consider using (flyspell-prog-mode).

Language Features

  • Don't use "inline" unless you're doing something so performance critical that the function call overhead will make a difference -- in other words: almost never. It makes debugging harder.
  • Use typedef carefully...
    • Do not create a new integer typedef without a really good reason.
    • Always postfix typedef names with _t so that they can be identified clearly in the code.
    • Never typedef pointers. The * makes C pointer declarations obvious - hiding it obfuscates the code.
  • Variable should be declared one per line, type and name, even if there are multiple variables of the same type. For maximum readability, the names should be aligned on the same column.
  right:

  int           len;
  int           count;
  struct inode *inode;
 
  wrong:

  int len, count;
  struct inode *inode;
 
  • Even for short conditionals, the operation should be on a separate line:
  if (foo)
          bar();
 
  • When you wrap, the next line should start after the parenthesis:
  right:

  variable = do_something_complicated(long_argument, longer_argument,
                                      longest_argument(sub_argument,
                                                       foo_argument),
                                      last_argument);

  if (some_long_condition(arg1, arg2, arg3) < some_long_value &&
      another_long_condition(very_long_argument_name,
                             another_long_argument_name) >
      second_long_value) {
  }
 
  wrong:

  variable = do_something_complicated(long_argument, longer_argument,
                    longest_argument(sub_argument, foo_argument),
                    last_argument);

  if (some_long_condition(arg1, arg2, arg3) < some_long_value &&
              another_long_condition(very_long_argument_name,
                    another_long_argument_name) >
                    second_long_value) {
  }
 
  • If you're wrapping put the operators at the end of the line, and if there are no parentheses indent 8 more:
  off = le32_to_cpu(fsd->fsd_client_start) +
          cl_idx * le16_to_cpu(fsd->fsd_client_size);
 
  • Binary and ternary (but not unary) operators should be separated from their arguments by one space.
  a++;
  b |= c;
  d = f > g ? 0 : 1;
 
  • Function calls should be nestled against the parentheses, the parentheses should crowd the arguments, and one space after commas:
  right: do_foo(bar, baz);
  wrong: do_foo ( bar,baz );
  • All if, for, while, etc. expressions should be separated by a space from the parenthesis, one space after the semicolons:
  for (a = 0; a < b; a++)
  if (a < b || a == c)
  while (1)
 
  • Opening braces should be on the same line as the line that introduces the block, except for function calls. Closing braces get their own line, except for "else".
  int foo(void)
  {
          if (bar) {
                  this();
                  that();
          } else if (baz) {
                  ;
          } else {
                  ;
          }
          do {
                  cow();
          } while (0);
  }
  • If one part of a compound if block has braces, all should.
  right:

  if (foo) {
          bar();
          baz();
  } else {
          salmon();
  }
  wrong:

  if (foo) {
          bar();
          baz();
  } else
          moose();
  • When you make a macro, protect those who might call it by using do/while and parentheses; line up your backslashes:
  right:

  #define DO_STUFF(a)                              \
  do {                                             \
          int b = (a) + MAGIC;                     \
          do_other_stuff(b);                       \
  } while (0)
  wrong:

  #define DO_STUFF(a) \
  { \
          int b = a + MAGIC; \
          do_other_stuff(b); \
  }
  • If you nest preprocessor commands, use spaces to visually delineate:
  #ifdef __KERNEL__
  # include <goose>
  # define MOOSE steak
  #else
  # include <mutton>
  # define MOOSE prancing
  #endif
  • For very long #ifdefs include the conditional with each #endif to make it readable:
  #ifdef __KERNEL__
  # if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)
  /* lots
     of
     stuff */
  # endif /* KERNEL_VERSION(2,5,0) */
  #else /* !__KERNEL__ */
  # if HAVE_FEATURE
  /* more
   * stuff */
  # endif
  #endif /* __KERNEL__ */
  • Comments should have the leading /* on the same line as the comment, and the trailing */ at the end of the last comment line. Intermediate lines should start with a * aligned with the first line's *:
  /* This is a short comment */

  /* This is a multi-line comment.  I wish the line would wrap already,
   * as I don't have much to write about. */
  • Conditional expressions read more clearly if boolean expressions are implicit, but non-boolean and pointer expressions compare explicitly with 0 and NULL respectively - e.g...
   right:

   if (inode != NULL &&     /* valid inode? */
       !writing &&          /* not writing? */
       ref_count == 0)      /* no more references? */
           do_this();
   wrong:

   if (inode &&             /* valid inode? */
       writing == 0 &&      /* not writing? */
       !ref_count)          /* no more references? */
           do_this();
  • Parentheses and line breaks help make conditional expressions more readable.
   right:

   if (a->a_field == 3 ||
       ((b->b_field & BITMASK1) && (c->c_field & BITMASK2)))
           do this();
   wrong:

   if (a->a_field == 3 || b->b_field & BITMASK1 && c->c_field & BITMASK2)
            do this()
  • Function declarations absolutely should NOT go into .c files, unless they are forward declarations for static functions that can't otherwise be moved before the caller. Instead, the declaration should go into the most "local" header available (preferrably *_internal.h for a given piece of code).
  • Structure and constant declarations should not be declared in multiple places. Put the struct into the most "local" header possible. If it is something that is passed over the wire it needs to go into lustre_idl.h, and needs to be correctly swabbed when the RPC message is unpacked.
  • The types and printf/printk formats used by Lustre code are:
   __u64                 LPU64/LPX64/LPD64 (unsigned, hex, signed)
   size_t                LPSZ (or cast to int and use %u / %d)
   __u32/int             %u/%x/%d (unsigned, hex, signed)
   (unsigned) long long  %llu/%llx/%lld
   loff_t                %lld after a cast to long long (unfortunately)
     AC_CACHE_CHECK([for EMX OS/2 environment], [ac_cv_emxos2],
     [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [return __EMX__;])],
                        [ac_cv_emxos2=yes],
                        [ac_cv_emxos2=no])])
or even
     AC_CACHE_CHECK([for EMX OS/2 environment],
                    [ac_cv_emxos2],
                    [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([],
                                                        [return __EMX__;])],
                                       [ac_cv_emxos2=yes],
                                       [ac_cv_emxos2=no])])