Re: CVS commit: src/share/misc

[Taylor R Campbell <campbell+netbsd-source-changes-d%mumble.net@localhost>]

> Module Name:    src
> Committed By:   lukem
> Date:           Sat Aug  1 02:45:36 UTC 2020
> 
> Modified Files:
>         src/share/misc: style
> 
> Log Message:
> style: prefer braces for single statement control statements
> 
> Prefer to use { braces } around single statements after
> control statements, instead of discouraging them.
> 
> Per discussion on tech-userlevel & tech-kern, where the significant
> majority of developers who responded (including current and former
> core members) prefer this new style.

Hmm...that's not the conclusion I got from the thread.  What you proposed
(https://mail-index.netbsd.org/tech-userlevel/2020/07/12/msg012536.html),
and got consensus on, was:

- discourage braces around single statements
+ permit braces around single statements

What you committed was:

- discourage braces around single statements
+ prefer braces around single statements and add braces to all examples

At least two core members (me and kre) preferred the change you
originally proposed over the change you committed.

Personally I feel that braces around short statements hurt legibility
by adding unnecessary visual clutter, and make it more cumbersome to
have consistent patterns like

	if (foo() == -1)
		goto fail0;
	if ((x = bar()) == -1)
		goto fail1;
	if (baz() == -1)
		goto fail2;

which makes it more tempting to get clever with shortcuts for error
branches or with reversing the sense of the branch, and we have too
many bugs with clever shortcuts in error branches already.

We don't have a `goto fail' problem in NetBSD -- if we did, our
toolchain would detect it with -Werror=misleading-indentation, as I
just confirmed experimentally.  (Same goes for macros that expand to
multiple statements, with -Werror=multistatement-macros.)

Can you please restore this to the change you originally suggested,
along the lines of the attached patch?

Index: share/misc/style
===================================================================
RCS file: /cvsroot/src/share/misc/style,v
retrieving revision 1.56
diff -p -p -u -r1.56 style
--- share/misc/style	1 Aug 2020 02:45:35 -0000	1.56
+++ share/misc/style	1 Aug 2020 22:54:53 -0000
@@ -241,9 +241,8 @@ main(int argc, char *argv[])
 			errno = 0;
 			num = strtol(optarg, &ep, 10);
 			if (num <= 0 || *ep != '\0' || (errno == ERANGE &&
-			    (num == LONG_MAX || num == LONG_MIN)) ) {
+			    (num == LONG_MAX || num == LONG_MIN)) )
 				errx(1, "illegal number -- %s", optarg);
-			}
 			break;
 		case '?':
 		default:
@@ -256,16 +255,16 @@ main(int argc, char *argv[])
 
 	/*
 	 * Space after keywords (while, for, return, switch).
-	 * Braces are preferred for control statements
-	 * with only a single statement.
+	 *
+	 * Braces around single-line bodies are optional; use discretion.
 	 *
 	 * Forever loops are done with for's, not while's.
 	 */
-	for (p = buf; *p != '\0'; ++p) {
+	for (p = buf; *p != '\0'; ++p)
 		continue;		/* Explicit no-op */
-	}
 	for (;;) {
-		stmt;
+		stmt1;
+		stmt2;
 	}
 
 	/*
@@ -317,9 +316,8 @@ main(int argc, char *argv[])
 	}
 
 	/* No spaces after function names. */
-	if ((result = function(a1, a2, a3, a4)) == NULL) {
+	if ((result = function(a1, a2, a3, a4)) == NULL)
 		exit(1);
-	}
 
 	/*
 	 * Unary operators don't require spaces, binary operators do.
@@ -397,12 +395,10 @@ function(int a1, int a2, float fl, int a
 	 *
 	 * Use err/warn(3), don't roll your own!
 	 */
-	if ((four = malloc(sizeof(*four))) == NULL) {
+	if ((four = malloc(sizeof(*four))) == NULL)
 		err(1, NULL);
-	}
-	if ((six = (int *)overflow()) == NULL) {
+	if ((six = (int *)overflow()) == NULL)
 		errx(1, "Number overflowed.");
-	}
 
 	/* No parentheses are needed around the return value. */
 	return eight;
@@ -426,9 +422,8 @@ dirinfo(const char *p, struct stat *sb, 
 	_DIAGASSERT(p != NULL);
 	_DIAGASSERT(filedesc != -1);
 
-	if (stat(p, sb) < 0) {
+	if (stat(p, sb) < 0)
 		err(1, "Unable to stat %s", p);
-	}
 
 	/*
 	 * To printf quantities that might be larger than "long", include