[libvirt] why we should use a mechanical indentation checker
Jim Meyering
jim at meyering.net
Tue May 4 13:43:46 UTC 2010
Jim Meyering wrote:
...
>> 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 at redhat.com>
> Date: Thu, 15 Apr 2010 19:31:04 +0200
> Subject: [PATCH] docs: hacking: explain why using curly braces well is important
That was incomplete. Didn't include the HACKING changes
and would provoke "make syntax-check" failure due to the
use of _("... making the po-check rule think hacking.html.in
should be listed in po/POTFILES.in.
Here's the patch that passes the tests:
>From 44258473b852ef6b8d87ad43c706b1d4f697fe4b Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering at 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.
---
HACKING | 91 ++++++++++++++++++++++++++++++++++
docs/hacking.html.in | 133 ++++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 215 insertions(+), 9 deletions(-)
diff --git a/HACKING b/HACKING
index 5486d8e..e22d73c 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 03a1bee..deab530 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/ > libvirt-myfeature.patch
-</pre>
+ diff -urp libvirt.orig/ libvirt.modified/ > 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)
+ die ("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
More information about the libvir-list
mailing list