Re: CVS commit: src/sys/arch/x86

2020-08-01 Thread Taylor R Campbell
> Module Name:src
> Committed By:   jdolecek
> Date:   Sat Aug  1 12:36:36 UTC 2020
> 
> Modified Files:
> src/sys/arch/x86/pci: pci_intr_machdep.c
> src/sys/arch/x86/x86: mainbus.c
> 
> Log Message:
> reorder includes to pull __HAVE_PCI_MSI_MSIX properly via
> 

If  requires a file to be included
("opt_pci.h"?) in order for its definition of __HAVE_PCI_MSI_MSIX to
work, why doesn't  include that file
directly?

Or, if I didn't follow exactly what's going on here, why is it
necessary to reorder the includes?

I think we should generally treat it as a bug if including two header
files in different orders gives different outcomes, and fix it by
fixing the header files rather than by adjusting the ordering of
includes in every file where they're used.


Re: CVS commit: src/share/misc

2020-08-01 Thread Luke Mewburn
On 20-08-01 23:07, Taylor R Campbell wrote:
  | 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/style1 Aug 2020 02:45:35 -   1.56
  | +++ share/misc/style1 Aug 2020 22:54:53 -
  | @@ -241,9 +241,8 @@ main(int argc, char *argv[])
  | errno = 0;
  | num = strtol(optarg, , 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;

IMO, that example is a case in where if the style is "minimal braces"
that's still a good case to retain it. The if condition is across
multiple lines, and the brakes make it clearer (to me) where the statement
is.

This all comes down to a matter of style, where some people
prefer "purple" and some prefer "orange", yet the arguments are
often claimed to be (by all concerned) as "technical reasons".

Anyway, I haven't got the motivation to bikeshed like this in NetBSD anymore.

Luke.


Re: CVS commit: src/share/misc

2020-08-01 Thread Luke Mewburn
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/style1 Aug 2020 02:45:35 -   1.56
  | +++ share/misc/style1 Aug 2020 22:54:53 -
  | @@ -241,9 +241,8 @@ main(int argc, char *argv[])
  | errno = 0;
  | num = strtol(optarg, , 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.


Re: CVS commit: src/sys/kern

2020-08-01 Thread Taylor R Campbell
> Module Name:src
> Committed By:   jdolecek
> Date:   Sat Aug  1 11:18:26 UTC 2020
> 
> Modified Files:
> src/sys/kern: subr_autoconf.c
> 
> Log Message:
> avoid VLA for the sizeof() calculations

Why?


Re: CVS commit: src/share/misc

2020-08-01 Thread Taylor R Campbell
> 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/style1 Aug 2020 02:45:35 -   1.56
+++ share/misc/style1 Aug 2020 22:54:53 -
@@ -241,9 +241,8 @@ main(int argc, char *argv[])
errno = 0;
num = strtol(optarg, , 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