Date: Mon, 23 Mar 2015 09:53:30 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: "Pedro F. Giffuni" <pfg@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r280346 - head/sys/sys Message-ID: <20150323084500.C1130@besplex.bde.org> In-Reply-To: <201503221537.t2MFb6Kf057971@svn.freebsd.org> References: <201503221537.t2MFb6Kf057971@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <machine/varargs> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150323084500.C1130>