Re: CVS commit: src/share/misc

[Luke Mewburn <lukem%NetBSD.org@localhost>]

On 20-08-01 23:07, Taylor R Campbell wrote:
  | > 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


I've reverted the change.

Bikeshed away.