[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] why we should use a mechanical indentation checker



Jim Meyering wrote:
> I introduced a bug with this supposedly-safe, no-semantic-change delta:
>
>     http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=b6719eab9e95c5daeeea85
>
> See if you can spot it.
>
> Here is the loop in question, as it was prior to my erroneous change:
>
>     for (i = 0; i < nruleInstances; i++)
>         switch (inst[i]->ruleType) {
>         case RT_EBTABLES:
>             ebiptablesInstCommand(&buf,
>                                   inst[i]->commandTemplate,
>                                   'A', -1, 1);
>         break;
>         case RT_IPTABLES:
>             haveIptables = true;
>         break;
>         case RT_IP6TABLES:
>             haveIp6tables = true;
>         break;
>         }
>
> Its formatting is WRONG.
...
> I am adding something like the following to coreutils' HACKING,
> and will add the equivalent (adjusting indentation/brace-positioning style
> in the examples) to libvirt's hacking.html.in, so if you object,
> speak up soon.
>
> While writing the above and below,
> I noticed that with the GNU formatting style,
> this is less of a problem, since there you do not add those
> oh-so-important-to-semantics curly braces at the *end* of a line
> where it easy to miss them, but at the beginning.  In addition,
> with the GNU style, adding the opening curly brace changes the
> indentation level of the body, while in libvirt's style, it does not.
>
> In either case, an automatic indentation checker would catch the problem.
> This is yet another reason to enforce an indentation style.
> The longer we wait, the harder it will become.

No one objected to the patch in the parent,
and there was one ACK, so I'm about to push this change
which has essentially the same content
(the formatting tweaks make html2text + massaging
produce something slightly closer to HACKING)

>From 8cd233784c6f85b6de00d2229d3bf4c42f9940ed Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering redhat com>
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.
---
 docs/hacking.html.in |  133 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 124 insertions(+), 9 deletions(-)

diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index 03a1bee..c678fb3 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -11,16 +11,15 @@
         early and listen to feedback.</li>

       <li><p>Post patches in unified diff format.  A command similar to this
-          should work:</p>
+        should work:</p>
         <pre>
-  diff -urp libvirt.orig/ libvirt.modified/ &gt; libvirt-myfeature.patch
-</pre>
+  diff -urp libvirt.orig/ libvirt.modified/ &gt; libvirt-myfeature.patch</pre>
         <p>
           or:
         </p>
         <pre>
-  git diff > libvirt-myfeature.patch
-</pre></li>
+  git diff > libvirt-myfeature.patch</pre>
+      </li>
       <li>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.</li>
@@ -29,16 +28,14 @@
       <li><p>Run the automated tests on your code before submitting any changes.
           In particular, configure with compile warnings set to -Werror:</p>
         <pre>
-  ./configure --enable-compile-warnings=error
-</pre>
+  ./configure --enable-compile-warnings=error</pre>
         <p>
           and run the tests:
         </p>
         <pre>
   make check
   make syntax-check
-  make -C tests valgrind
-</pre>
+  make -C tests valgrind</pre>
         <p>
           The latter test checks for memory leaks.
         </p>
@@ -60,6 +57,7 @@
 	<pre>
   ./qemuxml2xmltest</pre>

+      </li>
       <li>Update tests and/or documentation, particularly if you are adding
         a new feature or changing the output of a program.</li>
     </ol>
@@ -124,6 +122,123 @@
     </p>


+    <h2><a name="curly_braces">Curly braces</a></h2>
+
+    <p>
+      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.
+    </p>
+    <p>
+      Omitting braces with a single-line body is fine:
+    </p>
+
+    <pre>
+  while (expr) // one-line body -> omitting curly braces is ok
+      single_line_stmt ();</pre>
+
+    <p>
+      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:
+    </p>
+
+    <pre>
+  while (true) // BAD! multi-line body with no braces
+      /* comment... */
+      single_line_stmt ();</pre>
+    <p>
+      Do this instead:
+    </p>
+    <pre>
+  while (true) { // Always put braces around a multi-line body.
+      /* comment... */
+      single_line_stmt ();
+  }</pre>
+    <p>
+      There is one exception: when the second body line is not at the same
+      indentation level as the first body line:
+    </p>
+    <pre>
+  if (expr)
+      error (0, 0, _("a diagnostic that would make this line"
+                     " extend past the 80-column limit"));</pre>
+
+    <p>
+      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.
+    </p>
+
+    <p>
+      To reiterate, don't do this:
+    </p>
+
+    <pre>
+  if (expr)            // BAD: no braces around...
+      while (expr_2) { // ... a multi-line body
+          ...
+      }</pre>
+
+    <p>
+      Do this, instead:
+    </p>
+
+    <pre>
+  if (expr) {
+      while (expr_2) {
+          ...
+      }
+  }</pre>
+
+    <p>
+      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.
+    </p>
+
+    <pre>
+  if (expr) {
+      ...
+      ...
+  }
+  else
+      x = y;    // BAD: braceless "else" with braced "then"</pre>
+
+    <p>
+      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:
+    </p>
+
+    <pre>
+  if (!expr)
+    x = y; // putting the smaller block first is more readable
+  else {
+      ...
+      ...
+  }</pre>
+
+    <p>
+      If you'd rather not negate the condition, then at least add braces:
+    </p>
+
+    <pre>
+  if (expr) {
+      ...
+      ...
+  } else {
+      x = y;
+  }</pre>
+
     <h2><a href="types">Preprocessor</a></h2>

     <p>
--
1.7.1.335.g6845a


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]