Date: Fri, 01 May 2009 08:09:27 -0600 (MDT) From: "M. Warner Losh" <imp@bsdimp.com> To: christoph.mallon@gmx.de Cc: freebsd-hackers@FreeBSD.org Subject: Re: C99: Suggestions for style(9) Message-ID: <20090501.080927.-1923761682.imp@bsdimp.com> In-Reply-To: <49FA8D73.6040207@gmx.de> References: <20090428114754.GB89235@server.vk2pj.dyndns.org> <20090430.090226.1569754707.imp@bsdimp.com> <49FA8D73.6040207@gmx.de>
next in thread | previous in thread | raw e-mail | index | archive | help
In message: <49FA8D73.6040207@gmx.de>
Christoph Mallon <christoph.mallon@gmx.de> writes:
: M. Warner Losh schrieb:
: > In message: <20090428114754.GB89235@server.vk2pj.dyndns.org>
: > Peter Jeremy <peterjeremy@optushome.com.au> writes:
: > : >Maybe using all of these changes is a bit too big change at once=
, but =
: > : >I'd like some of them applied to style(9). So, please consider t=
he =
: > : >proposed changes individually and not a as a all-or-nothing pack=
age.
: > : =
: > : One area you do not address is code consistency. Currently, the
: > : FreeBSD kernel (and, to a lesser extent, userland) are mostly sty=
le(9)
: > : compliant - ie the entire codebase is mostly written in the same
: > : style. Whilst you may not like it (and I don't know that anyone
: > : completely agrees with everything in style(9)), it does mean that=
: > : the code is consistent.
: > : =
: > : Changing style(9) in a way that is not consistent with current co=
de
: > : means that either existing code must be brought into line with th=
e
: > : new standard - which incurs a one-off cost - or the code base bec=
omes
: > : split into "old" and "new" style - incurring an ongoing maintenan=
ce
: > : cost as maintainers switch between styles. Both approaches incur=
a
: > : risk of introducing new bugs.
: > : =
: > : Note that I'm not saying that style(9) can never be changed, I'm =
just
: > : saying that any change _will_ incur a cost and the cost as well a=
s
: > : the potential benefits need to be considered.
: > =
: > This is my biggest worry about changing style(9) quickly. We don't=
: > want needless churn in the tree for just style changes since it mak=
es
: > it harder to track bug fixes into the past.
: =
: I'm not saying that all the code has to be changed at once, but no ne=
w =
: code of the "old" style should be added. E.g. you should never use K&=
R =
: declarations when adding a new function. And if you are going to make=
a =
: big change somewhere, then it is sensible to use the "new" style.
Mixing and matching style has proven to be bad when it has been
practiced in the past. At least when practiced on a sub-file level.
: > : [Reduce declaration scope as far as possible, initialise variable=
s where
: > : they are declared, sort by name]
: > : =
: > : Whilst my personal preference is for the style suggested by Chris=
toph
: > : (and I generally agree with the points he makes), this is also th=
e
: > : change that incurs the biggest stylistic change. This is not a c=
hange
: > : that can be practically retrofitted to existing code and therefor=
e its
: > : implementation would result in a mixture of code styles - increas=
ing
: > : the risk of bugs due to confusion as to which style was being use=
d. I
: > : am not yet sure whether the benefits outweigh the costs,
: > =
: > This is the biggest one, and I think it may be too soon. Also, we
: =
: This is the reason, why I explicitly mentioned, that the proposed =
: changes should be examined independently.
: =
: > need to be careful on the initialization side of things because we
: > currently have a lot of code that looks like:
: > =
: > =
: > struct foo *fp;
: > struct bar *bp;
: > =
: > fp =3D get_foo();
: > if (!fp) return;
: > bp =3D fp->bp;
: > =
: > this can't easily be translated to the more natural:
: > =
: > struct foo *fp =3D get_foo();
: > struct bar *bp =3D fp->bp;
: > =
: > since really you'd want to write:
: > =
: > struct foo *fp =3D get_foo();
: > if (!fp) return;
: > struct bar *bp =3D fp->bp;
: > =
: > which isn't legal in 'C'. However, we have enough where this isn't=
: =
: You're mistaken, this is perfectly legal C. See ISO/IEC 9899:1999 (E)=
: =A76.8.2:1. In short: you can mix statements and declarations.
: =
: > the case that it should be allowed. We should separate the
: > initialization part of this from the scope part of this too. The
: > initialization part is already leaking into the code, while instanc=
es
: > of the scope restriction is rarer.
: =
: Sorry, I do not understand these sentences.
It's a stupid idea and I don't think we should do it. It is a bad
practice to intermix statements and declarations, and I think a too
radical departure from the current style.
: > : [Don't parenthesize return values]
: > : >Removed, because it does not improve maintainability in any way.=
: > : =
: > : This change could be made and tested mechanically. But there is
: > : no justification for making it - stating that the existing rule
: > : is no better than the proposed rule is no reason to change.
: > =
: > I'm fine with this.
: > =
: > : [No K&R declarations]
: > : >Removed, because there is no need to use them anymore.
: > : =
: > : Whilst this is a change that could be performed mechanically, the=
re
: > : are some gotchas relating to type promotion (as you point out).
: > : The kernel already contains a mixture of ANSI & K&R declarations =
and
: > : style(9) recommends using ANSI. IMHO, there is no need to make t=
his
: > : change until all K&R code is removed from FreeBSD.
: > =
: > I'm fine with this change.
: > =
: > : [ Don't insert an empty line if the function has no local variabl=
es.]
: > : =
: > : This change could be made and tested mechanically. IMHO, this ch=
ange
: > : has neglible risk and could be safely implemented.
: > =
: > I'm fine with this change.
: =
: Would you commit these three proposals?
Yes.
: > : >> +.Sh LOCAL VARIABLES
: > : =
: > : >Last, but definitely not least, I added this paragraph about the=
use of =
: > : >local variables. This is to clarify, how today's compilers handl=
e =
: > : >unaliased local variables.
: > : =
: > : Every version of gcc that FreeBSD has ever used would do this for=
: > : primitive types when optimisation was enabled. This approach can=
: > : become expensive in stack (and this is an issue for kernel code) =
when
: > : using non-primitive types or when optimisation is not enabled (th=
ough
: > : I'm not sure if this is supported). Assuming that gcc (and icc a=
nd
: > : clang) behaves as stated in all supported optimisation modes, thi=
s
: > : change would appear to be quite safe to make.
: > =
: > Agreed, in general. We don't want to optimize our code style based=
on
: > what one compiler does, perhaps on x86.
: =
: I'm not sure whether I understand this - in particular the last three=
words.
I'm saying we shouldn't tune our coding standard to the optimizations
that the compiler of the hour gives, especially if those optimizations
to the style are tuned to one architecture. Since there's little
evidence presented on how these style changes will help any
architecture, it is hard to judge if this is the case or not.
Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090501.080927.-1923761682.imp>