[libvirt] why we should use a mechanical indentation checker

15 Apr 2010

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.
Sure, it is technically correct to omit the curly braces
when the for-loop body is a single *statement*, as it is above.
However, the moment the construct in the body occupies
more than one line, it is very risky to omit the curly braces.

I demonstrated that yesterday by inserting a no-op sa_assert
statement as the "first" line of that loop body, not noticing
that unlike my other two additions, this one pushed the
single-stmt body *out of the loop*.  Nasty.

For example, do not omit the curly braces even when the body is
just a single-line statement but with a preceding comment.

    while (true)
        /* explanation... */                  BAD!
        single_line_stmt ();

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


Of course do not do this, either:

    if (expr)
        while (other_expr) {                  BAD!
            ...
        }

Do this, instead:

    if (expr) {
        while (other_expr) {
            ...
        }
    }

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.

Note: these diffs are relative to coreutils' HACKING file, not libvirt's:

diff --git a/HACKING b/HACKING
index 124c666..4a6d593 100644
--- a/HACKING
+++ b/HACKING
@@ -233,6 +233,110 @@ Try to make the summary line fit one of the following forms:
   maint: change-description


+Curly braces: use judiciously
+=============================
+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.
+
+For example, do not omit the curly braces even when the body is just a
+single-line statement but with a preceding comment.
+
+Omitting braces with a single-line body is fine:
+
+     while (expr)
+       single_line_stmt ();
+
+However, the moment your loop 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)
+       /* comment... */      // BAD: multi-line body without braces
+       single_line_stmt ();
+
+Do this instead:
+
+     while (true)
+       {  /* Always put braces around a multi-line body.  */
+         /* explanation... */
+         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)
+       error (0, 0, _("a diagnostic that would make this line"
+                      " extend past the 80-column limit"));
+
+It seems safe not to require curly braces in this case,
+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)
+       while (expr_2)        // BAD: multi-line body without braces
+         {
+           ...
+         }
+
+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;                  /* more readable */
+    else
+      {
+        ...
+        ...
+      }
+
+If you'd rather not negate the condition, then add braces:
+
+    if (expr)
+      {
+        ...
+        ...
+      }
+    else
+      {
+        x = y;
+      }
+
+
 Use SPACE-only indentation in all[*] files
 ==========================================
 We use space-only indentation in nearly all files.

    

Jim Meyering

Eric Blake

Jim Meyering

Jim Meyering

tags

participants (2)