svn commit: r294246 - user/ngie/socket-tests/tools/regression/sockets/unix_cmsg

[Bruce Evans]

On Mon, 18 Jan 2016, Garrett Cooper wrote:

> Log:
>  Checkpoint work to bump WARNS to 6
>
>  Try to fix -Wcast-align issues by using bcopy to assign the values in
>  the structure.

Using the standard memmove() instead of the BSD bcopy() might be considered
a style bug.  However, this is a case where memmove() works better.  The
compiler normally replaces it by __builtin_mmemove() which can be optimized
better for small fixed-size copies.  On arches without strict alignment
requirements, this normally results in the same loads and stores that the
pointer hack would give.

However2, the kernel build environment is not normal, and changes
related to this give a mess of style bugs and negative optimizations
instead of the indended positive ones.  Long ago, the style was broken
by changing some bcopy()s (mainly in networking code) to memcpy()s and
adding a fallback extern memcpy() for compilers that don't do the
optimization (mainly for the -O0 case).  Before that, it was easier
to enforce the style by not supporting memcpy().  This was turned into
less than a style bug by adding -ffreestanding to CFLAGS (mainly to
fix the compiler replacing printf() by __builtin_printf() and turning
that into puts() in some cases).  -ffreestanding is technically correct,
but it disables _all_ builtins.  This made the changes to use
__builtin_memcpy() less than style bugs since the fallback memcpy()
didn't attempt to be optimal.  However, it is more optimal than bcopy()
for small copies on some arches.  No one cares about the lost builtins
since the optimizations that they give are tiny.

Later, many more mem*() functions were added, mainly to support contribed
software that doesn't follow BSD style.  The result is a mess of different
styles in non-contrib'ed code too.

> ==============================================================================
> --- user/ngie/socket-tests/tools/regression/sockets/unix_cmsg/unix_cmsg.c	Mon Jan 18 04:24:43 2016	(r294245)
> +++ user/ngie/socket-tests/tools/regression/sockets/unix_cmsg/unix_cmsg.c	Mon Jan 18 04:32:19 2016	(r294246)
> @@ -1027,60 +1027,60 @@ check_xucred(const struct xucred *xucred
> static int
> check_scm_creds_cmsgcred(struct cmsghdr *cmsghdr)
> {
> -	const struct cmsgcred *cmsgcred;
> +	struct cmsgcred cmsgcred;

The changes fix many style bugs (pointer variables not spelled with
a p) but leave many others (verbose variable names).

> @@ -1145,15 +1145,15 @@ check_scm_creds_sockcred(struct cmsghdr
> static int
> check_scm_timestamp(struct cmsghdr *cmsghdr)
> {
> -	const struct timeval *timeval;
> +	const struct timeval timeval;

'timeval' is a bad enough name for a struct tag.  'timeval' is a worse
variable name since it is the same as the struct tag.  The normal name
is 'tv'.

> @@ -1161,15 +1161,15 @@ check_scm_timestamp(struct cmsghdr *cmsg
> static int
> check_scm_bintime(struct cmsghdr *cmsghdr)
> {
> -	const struct bintime *bintime;
> +	const struct bintime bintime;

Names related to bintimes are generally worse than for timevals, starting
in the implementation not having prefixes in the struct member names.

Timespecs are in between.  They have prefixes in the struct member names,
but POSIX broke these from ts_ to tv_.

Bruce