From owner-svn-src-all@FreeBSD.ORG Sun Mar 22 22:53:41 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id F1CCFD8C; Sun, 22 Mar 2015 22:53:40 +0000 (UTC) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 9B1B4260; Sun, 22 Mar 2015 22:53:40 +0000 (UTC) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id 7B70F78119C; Mon, 23 Mar 2015 09:53:31 +1100 (AEDT) Date: Mon, 23 Mar 2015 09:53:30 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: "Pedro F. Giffuni" Subject: Re: svn commit: r280346 - head/sys/sys In-Reply-To: <201503221537.t2MFb6Kf057971@svn.freebsd.org> Message-ID: <20150323084500.C1130@besplex.bde.org> References: <201503221537.t2MFb6Kf057971@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=ZuzUdbLG c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=tY7LaoIE7Bbx0eQ_6PYA:9 a=CjuIK1q_8ugA:10 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 22 Mar 2015 22:53:41 -0000 On Sun, 22 Mar 2015, Pedro F. Giffuni wrote: > Log: > Small style(9) cleanup. > > #define should always be followed by a tab not space. Thanks, but not in comments. > Modified: head/sys/sys/cdefs.h > ============================================================================== > --- head/sys/sys/cdefs.h Sun Mar 22 13:11:56 2015 (r280345) > +++ head/sys/sys/cdefs.h Sun Mar 22 15:37:05 2015 (r280346) > @@ -70,20 +70,20 @@ > #if defined(__GNUC__) || defined(__INTEL_COMPILER) > > #if __GNUC__ >= 3 || defined(__INTEL_COMPILER) > -#define __GNUCLIKE_ASM 3 > -#define __GNUCLIKE_MATH_BUILTIN_CONSTANTS > +#define __GNUCLIKE_ASM 3 > +#define __GNUCLIKE_MATH_BUILTIN_CONSTANTS The commit that added the __INTEL_COMPILER stuff was well-intentioned but not ready for commit. The lines changed by it still have mounds of style bugs. They mostly still don't use tabs where they are more needed -- to line up the #defined values. The __INTEL_COMPILER stuff was only used in a few places for i386 originally (not even in the corresponding places for amd64), and has rotted. It is well intentioned, but mostly just makes the code uglier where it is actually used. > ... > #ifndef __INTEL_COMPILER > # define __GNUCLIKE_CTOR_SECTION_HANDLING 1 > #endif Indenting 'define' after '#' is another style bug. Apparently this defeated your pattern matcher, so the space after 'define' is not fixed here. > > -#define __GNUCLIKE_BUILTIN_CONSTANT_P 1 > +#define __GNUCLIKE_BUILTIN_CONSTANT_P 1 > # if defined(__INTEL_COMPILER) && defined(__cplusplus) \ > && __INTEL_COMPILER < 800 '&&' on a new line is another style bug. It is gnu-style. > @@ -111,19 +111,19 @@ > # define __GNUCLIKE_MATH_BUILTIN_RELOPS > #endif > > -#define __GNUCLIKE_BUILTIN_MEMCPY 1 > +#define __GNUCLIKE_BUILTIN_MEMCPY 1 > > /* XXX: if __GNUC__ >= 2: not tested everywhere originally, where replaced */ > -#define __CC_SUPPORTS_INLINE 1 > -#define __CC_SUPPORTS___INLINE 1 > -#define __CC_SUPPORTS___INLINE__ 1 > +#define __CC_SUPPORTS_INLINE 1 > +#define __CC_SUPPORTS___INLINE 1 > +#define __CC_SUPPORTS___INLINE__ 1 3 different macros to support the different spellings of 'inline' are excessive, and also wrong: - __inline__ is a gnu spelling. Any use of it is a style and portability bug. Using it defeats portability ifdefs in this file, since only the FreeBSD spelling __inline is really supported in this file. The macro __CC_SUPPORTS___INLINE__ to support it shouldn't exist. Currently, there are 256 lines in /sys with this style bug (mostly in mips, fuse, xen, drm and contrib) :-(. There are just 3 lines which reference the macro: - 1 in cdefs.h to define it - 1 in cx to do home made portability stuff (inline -> __inline__). The FreeBSD spelling of this portability stuff is __inline or just plain inline in sloppier code. Even the sloppier code became portable to new compilers with C99. - 1 in iir. iir uses __inline__ a lot, and just fails when the macro says that it is not supported. - __inline is the FreeBSD spelling. It is used a lot. About 3500 times in /sys. The macro shouldn't exist, since portability ifdefs in cdefs.h are supposed to make __inline usable for most cases, by defining it to nothing if required. These portability ifdefs are mostly for handling the difference between C90 and gcc in 1990, and aren't complicated enough to handle all 1990 model compilers, but most code didn't care about the difference even in 1990. There are just 5 uses of the macro (not counting comments or its definition): - 2 in cpufunc.h (1 each for amd64 and i386) - 3 in in_cksum.h (1 each for amd64, mips, and powerpc. i386 is gratuitously different from amd64 due to different bad fixes for bugs in the inline asm). - inline is the C99 spelling. It is used almost 6000 times (mostly in contrib'ed uglyware). The macro for it made sense before C99 existed, but was committed after 1999 and has never been used. > -#define __CC_SUPPORTS___FUNC__ 1 > -#define __CC_SUPPORTS_WARNING 1 > +#define __CC_SUPPORTS___FUNC__ 1 > +#define __CC_SUPPORTS_WARNING 1 __func__ is like __inline, but has better compatibility support, so the macro for it should not exist. The macro is used just once (in ichsmb, for just 1 of 12 instances of __func__ in this file). > > -#define __CC_SUPPORTS_VARADIC_XXX 1 /* see varargs.h */ > +#define __CC_SUPPORTS_VARADIC_XXX 1 /* see varargs.h */ > > -#define __CC_SUPPORTS_DYNAMIC_ARRAY_INIT 1 > +#define __CC_SUPPORTS_DYNAMIC_ARRAY_INIT 1 The features corresponding to these macros are much more arcane than __inline and __func__. __CC_SUPPORTS_VARADIC_XXX is used just once (in i386 varags.h. Varargs is very compiler-dependent, but only the implementation should know the details, and is a better place to put them than here even if they only depend on the compiler). __CC_SUPPORTS_VARADIC_XXX is not used. (All counts in /sys). > ... > @@ -139,7 +139,7 @@ > > /* > * The __CONCAT macro is used to concatenate parts of symbol names, e.g. > - * with "#define OLD(foo) __CONCAT(old,foo)", OLD(foo) produces oldfoo. > + * with "#define OLD(foo) __CONCAT(old,foo)", OLD(foo) produces oldfoo. Example of a comment mangled by the substitution. > @@ -234,13 +234,13 @@ > #define __section(x) __attribute__((__section__(x))) > #endif > #if defined(__INTEL_COMPILER) > -#define __dead2 __attribute__((__noreturn__)) > -#define __pure2 __attribute__((__const__)) > -#define __unused __attribute__((__unused__)) > -#define __used __attribute__((__used__)) > -#define __packed __attribute__((__packed__)) > -#define __aligned(x) __attribute__((__aligned__(x))) > -#define __section(x) __attribute__((__section__(x))) > +#define __dead2 __attribute__((__noreturn__)) > +#define __pure2 __attribute__((__const__)) > +#define __unused __attribute__((__unused__)) > +#define __used __attribute__((__used__)) > +#define __packed __attribute__((__packed__)) > +#define __aligned(x) __attribute__((__aligned__(x))) > +#define __section(x) __attribute__((__section__(x))) > #endif > #endif __INTEL_COMPILER causes complications by not being fully gcc-compatible, but here it is fully compatible and the duplication is spam. Previously, the spam wasn't even duplication since it had corrupt tabs. Duplication defeats the point of complicated ifdefs elsewhere, where the complications go in the direction of putting a single definition under a complicated condition. clang would require much messier ifdefs if it were not close to 100% compatible, since it is actually used. Bruce