From owner-freebsd-bugs Mon Nov 23 09:59:56 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id JAA12826 for freebsd-bugs-outgoing; Mon, 23 Nov 1998 09:59:56 -0800 (PST) (envelope-from owner-freebsd-bugs@FreeBSD.ORG) Received: from freefall.freebsd.org (freefall.FreeBSD.ORG [204.216.27.21]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id JAA12821 for ; Mon, 23 Nov 1998 09:59:55 -0800 (PST) (envelope-from gnats@FreeBSD.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.8.8/8.8.5) id KAA10433; Mon, 23 Nov 1998 10:00:01 -0800 (PST) Date: Mon, 23 Nov 1998 10:00:01 -0800 (PST) Message-Id: <199811231800.KAA10433@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.ORG From: Robert Nordier Subject: Re: kern/8821: "warning: suggest parentheses" fixes Reply-To: Robert Nordier Sender: owner-freebsd-bugs@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org The following reply was made to PR kern/8821; it has been noted by GNATS. From: Robert Nordier 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