[Swan-dev] readable C style for split control statements

[D. Hugh Redelmeier]

Tuomo just committed 8db3582c4cb021ce762c9832a5314d28018f10aa:
	addr_lookup.c: fix coding style

These changed indentations of IF statements that were split across lines.

For example:
@@ -181,7 +181,7 @@ static ssize_t netlink_read_reply(int sock, char **pbuf, size_t bufsize,
 		struct nlmsghdr *nlhdr = (struct nlmsghdr *)(*pbuf + msglen);
 
 		if (!NLMSG_OK(nlhdr, (size_t)readlen) ||
-		    nlhdr->nlmsg_type == NLMSG_ERROR)
+			nlhdr->nlmsg_type == NLMSG_ERROR)
 			return -1;
 
 		/* Move read pointer */


This absolutely brings these statements into conformity with
<https://github.com/torvalds/linux/blob/master/Documentation/process/coding-style.rst>

As I've argued before, we should not be following that rule.  It makes
the code harder to read.  I've quoted that message below.

I just checked on actual kernel code.  I did a grep in
linux-3.17.4/kernel to see what is actually done in the kernel.  I
picked 3.17.4 because that's what I happen to have laying about.  I
picked the kernel directory on the hunch that that's where the core
kernel developers work.  Here's the egrep:

egrep -nR -A 1 '\<if \(.........................................*[-a-zA-Z_.,<>(|&+*5]$'

What I found: only a small minority of split IF statements follow the
kernel style rule by indenting the continued line by a single
additional tab.  Many do what I recommend.  Many do something else.

So: even the kernel doesn't follow the style rules.  Why should we
follow them when they make the code less readable?

| From: D. Hugh Redelmeier <hugh at mimosa.com>
| To: Libreswan Development List <swan-dev at lists.libreswan.org>
| Date: Sun, 15 Jun 2014 14:08:01 -0400 (EDT)
| Subject: readable C style for split control statements
| 
| Here's a change that Tuomo just checked in.  This changes the
| indentation of the folded part of an if statement:
| 
|         if (conn->options_set[KBF_DPDDELAY] &&
|             conn->options_set[KBF_DPDTIMEOUT]) {
|                 msg.dpd_delay = deltatime(conn->options[KBF_DPDDELAY]);
|                 msg.dpd_timeout = deltatime(conn->options[KBF_DPDTIMEOUT]);
| =>
|         if (conn->options_set[KBF_DPDDELAY] &&
|                 conn->options_set[KBF_DPDTIMEOUT]) {
|                 msg.dpd_delay = deltatime(conn->options[KBF_DPDDELAY]);
|                 msg.dpd_timeout = deltatime(conn->options[KBF_DPDTIMEOUT]);
| 
| I read code from the left.  When I'm scanning the revised version, I
| start with the expectation that the second line is a statement within
| the then-part of the if.  Only when I get to the end do I find that it
| is actually more of the predicate.
| 
| When I read the original version, the distinct indentation makes it
| clear that the second line is part of the predicate.
| 
| We don't want to create
| <http://en.wikipedia.org/wiki/Garden_path_sentence>
| 
| I propose that each broken control-statement head be indented in such a way
| as to make the fragments visually distinct from the body.  This will
| require spaces for the last bit of the indentation.
| 
| Thus the version before the change is correct and the changed version is 
| wrong.