Date: Mon, 23 Nov 1998 10:00:01 -0800 (PST) From: Robert Nordier <rnordier@nordier.com> To: freebsd-bugs@FreeBSD.ORG Subject: Re: kern/8821: "warning: suggest parentheses" fixes Message-ID: <199811231800.KAA10433@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/8821; it has been noted by GNATS.
From: Robert Nordier <rnordier@nordier.com>
To: bradley@dunn.org
Cc: FreeBSD-gnats-submit@FreeBSD.ORG
Subject: Re: kern/8821: "warning: suggest parentheses" fixes
Date: Mon, 23 Nov 1998 19:49:24 +0200 (SAT)
bradley@dunn.org wrote:
> >Number: 8821
> >Category: kern
> >Synopsis: "warning: suggest parentheses" fixes
> The attached patches fix all "warning: suggest parentheses"
> in src/sys/dev
Of the various warnings issued by gcc, these are probably the most
questionable because (being purely stylistic suggestions) they may be
issued against code which is technically correct in every respect.
As applied to FreeBSD, the stylistic issue is not really whether the
gcc-suggested style is superior to the present one (it is certainly
arguable that it is). Rather, it is a matter of consistency with
existing practice.
The man page style(9) is an attempt to describe the preferred form
for FreeBSD kernel source files. An extract says:
| No spaces after function names. Commas have a space after them.
| No spaces after `(' or `[' or preceding `]' or `)' characters.
|
| if (error = function(a1, a2))
| exit(error);
|
| Unary operators don't require spaces, binary operators do.
| Don't use parentheses unless they're required for precedence,
| or the statement is really confusing without them.
The majority of the supplied fixes are, in fact, applied to
statements very similar to the quoted style(9) example; and are
therefore clearly not in accordance with the guidelines.
In other cases, an extra level of parentheses is added to '&' and
'|' operands. For example, one of the more complex cases has the
logical form
p & ~q == r << s
which is changed to
p & (~q == r << s)
It is doubtful that such as statement, in either form, qualifies
as "really confusing"; and this essentially disposes of the
other suggested changes.
Unfortunately, in a few cases, you have missed instances where
the warning may have prompted a worthwhile fix.
One example:
> --- src/sys/dev/ppbus/ppb_base.c.old Mon Nov 23 07:06:42 1998
> +++ src/sys/dev/ppbus/ppb_base.c Mon Nov 23 07:07:29 1998
> @@ -87,7 +87,7 @@
> case PPB_INTR:
> default:
> /* wait 10 ms */
> - if ((error = tsleep((caddr_t)dev, PPBPRI | PCATCH,
> + if ((error = tsleep((caddr_t)dev, (PPBPRI | PCATCH),
> "ppbpoll", hz/100)))
> return (error);
> break;
The real "problem" here is in src/sys/dev/ppbus/ppbconf.h, where
#define PPBPRI PZERO+8
should probably be changed to
#define PPBPRI (PZERO + 8)
since #defines in header files are almost always better off
fully-parenthesized.
But just sliding an extra layer of parentheses around the '|' (as in
the patch) achieves nothing, and should not even inhibit the compiler
from continuing to make its original "suggestion".
Apologies if this seems unduly negative. You just happen to have
chosen a category of warnings where it is almost certainly better to
ignore the compiler, rather than make substantial non-functional
source changes to quiet it.
--
Robert Nordier
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-bugs" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199811231800.KAA10433>