From 44258473b852ef6b8d87ad43c706b1d4f697fe4b Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Thu, 15 Apr 2010 19:31:04 +0200 Subject: [PATCH] docs: hacking: explain why using curly braces well is important * docs/hacking.html.in: Use the "curly braces" section from coreutils' HACKING, adapting for libvirt's different formatting style. * HACKING: Sync from the above, still mostly manually. --- HACKING | 91 +++++++++++++++++++++++++++++ docs/hacking.html.in | 133 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 215 insertions(+), 9 deletions(-) diff --git a/HACKING b/HACKING index 5486d8edcd..e22d73c71e 100644 --- a/HACKING +++ b/HACKING @@ -105,6 +105,97 @@ Usually they're in macro definitions or strings, and should be converted anyhow. +Curly braces +============ +Omit the curly braces around an "if", "while", "for" etc. body only when that +body occupies a single line. In every other case we require the braces. This +ensures that it is trivially easy to identify a single-*statement* loop: each +has only one *line* in its body. + +Omitting braces with a single-line body is fine: + + while (expr) // one-line body -> omitting curly braces is ok + single_line_stmt (); + +However, the moment your loop/if/else body extends onto a second line, for +whatever reason (even if it's just an added comment), then you should add +braces. Otherwise, it would be too easy to insert a statement just before that +comment (without adding braces), thinking it is already a multi-statement +loop: + + while (true) // BAD! multi-line body with no braces + /* comment... */ + single_line_stmt (); + +Do this instead: + + while (true) { // Always put braces around a multi-line body. + /* comment... */ + single_line_stmt (); + } + +There is one exception: when the second body line is not at the same +indentation level as the first body line: + + if (expr) + die ("a diagnostic that would make this line" + " extend past the 80-column limit")); + +It is safe to omit the braces in the code above, since the further-indented +second body line makes it obvious that this is still a single-statement body. + + +To reiterate, don't do this: + + if (expr) // BAD: no braces around... + while (expr_2) { // ... a multi-line body + ... + } + +Do this, instead: + + if (expr) { + while (expr_2) { + ... + } + } + +However, there is one exception in the other direction, when even a one-line +block should have braces. That occurs when that one-line, brace-less block is +an "else" block, and the corresponding "then" block *does* use braces. In that +case, either put braces around the "else" block, or negate the "if"-condition +and swap the bodies, putting the one-line block first and making the longer, +multi-line block be the "else" block. + + if (expr) { + ... + ... + } + else + x = y; // BAD: braceless "else" with braced "then" + +This is preferred, especially when the multi-line body is more than a few +lines long, because it is easier to read and grasp the semantics of an if- +then-else block when the simpler block occurs first, rather than after the +more involved block: + + if (!expr) + x = y; // putting the smaller block first is more readable + else { + ... + ... + } + +If you'd rather not negate the condition, then at least add braces: + + if (expr) { + ... + ... + } else { + x = y; + } + + Preprocessor ============ For variadic macros, stick with C99 syntax: diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 03a1bee17d..deab530abc 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -11,16 +11,15 @@ early and listen to feedback.

  • Post patches in unified diff format. A command similar to this - should work:

    + should work:

    -  diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch
-
    + diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch

    or: 

    -  git diff > libvirt-myfeature.patch
-
    • + git diff > libvirt-myfeature.patch +
  • Split large changes into a series of smaller patches, self-contained if possible, with an explanation of each patch and an explanation of how the sequence of patches fits together.
    • @@ -29,16 +28,14 @@

  • Run the automated tests on your code before submitting any changes. In particular, configure with compile warnings set to -Werror:

    -  ./configure --enable-compile-warnings=error
-
    + ./configure --enable-compile-warnings=error

    and run the tests: 

       make check
   make syntax-check
-  make -C tests valgrind
-
    + make -C tests valgrind

    The latter test checks for memory leaks.

    @@ -60,6 +57,7 @@ 
       ./qemuxml2xmltest
    +
  • Update tests and/or documentation, particularly if you are adding a new feature or changing the output of a program.
    • @@ -124,6 +122,123 @@

    +

    Curly braces

    + +

    + Omit the curly braces around an "if", "while", "for" etc. body only + when that body occupies a single line. In every other case we require + the braces. This ensures that it is trivially easy to identify a + single-*statement* loop: each has only one *line* in its body. +

    +

    + Omitting braces with a single-line body is fine: +

    + + 
    +  while (expr) // one-line body -> omitting curly braces is ok
+      single_line_stmt ();
    + +

    + However, the moment your loop/if/else body extends onto a second + line, for whatever reason (even if it's just an added comment), then + you should add braces. Otherwise, it would be too easy to insert a + statement just before that comment (without adding braces), thinking + it is already a multi-statement loop: +

    + + 
    +  while (true) // BAD! multi-line body with no braces
+      /* comment... */
+      single_line_stmt ();
    +

    + Do this instead: +

    + 
    +  while (true) { // Always put braces around a multi-line body.
+      /* comment... */
+      single_line_stmt ();
+  }
    +

    + There is one exception: when the second body line is not at the same + indentation level as the first body line: +

    + 
    +  if (expr)
+      die ("a diagnostic that would make this line"
+           " extend past the 80-column limit"));
    + +

    + It is safe to omit the braces in the code above, since the + further-indented second body line makes it obvious that this is still + a single-statement body. +

    + +

    + To reiterate, don't do this: +

    + + 
    +  if (expr)            // BAD: no braces around...
+      while (expr_2) { // ... a multi-line body
+          ...
+      }
    + +

    + Do this, instead: +

    + + 
    +  if (expr) {
+      while (expr_2) {
+          ...
+      }
+  }
    + +

    + However, there is one exception in the other direction, when even a + one-line block should have braces. That occurs when that one-line, + brace-less block is an "else" block, and the corresponding "then" block + *does* use braces. In that case, either put braces around the "else" + block, or negate the "if"-condition and swap the bodies, putting the + one-line block first and making the longer, multi-line block be the + "else" block. +

    + + 
    +  if (expr) {
+      ...
+      ...
+  }
+  else
+      x = y;    // BAD: braceless "else" with braced "then"
    + +

    + This is preferred, especially when the multi-line body is more than a + few lines long, because it is easier to read and grasp the semantics of + an if-then-else block when the simpler block occurs first, rather than + after the more involved block: +

    + + 
    +  if (!expr)
+    x = y; // putting the smaller block first is more readable
+  else {
+      ...
+      ...
+  }
    + +

    + If you'd rather not negate the condition, then at least add braces: +

    + + 
    +  if (expr) {
+      ...
+      ...
+  } else {
+      x = y;
+  }
    +

    Preprocessor