Date: Wed, 20 Jan 2016 22:21:14 -0800 From: NGie Cooper <yaneurabeya@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: Garrett Cooper <ngie@freebsd.org>, src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r294246 - user/ngie/socket-tests/tools/regression/sockets/unix_cmsg Message-ID: <347D22CE-6DDB-4AE9-879A-73743723B481@gmail.com> In-Reply-To: <20160118153831.K1354@besplex.bde.org> References: <201601180432.u0I4WJYa008708@repo.freebsd.org> <20160118153831.K1354@besplex.bde.org>
index | next in thread | previous in thread | raw e-mail
> On Jan 17, 2016, at 21:13, Bruce Evans <brde@optusnet.com.au> wrote:
>
> 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.
I tried memmove and unfortunately it failed similar to bcopy. I think I need to do “deeper copying” in the structures to ensure that all of the fields are properly copied over in the bcopy, but I’ll look at what gdb says first.
>> ==============================================================================
>> --- 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).
Agreed. I’ll fix this in the next commit.
>> @@ -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’.
I agree in these cases. Unless it’s standard variable naming convention for C (rc/retval/rv being shorthand for “return_value”, etc), I try to stick with more verbose names to make the meaning of variables unambiguous. I agree though… the variable name `timeval` is not incredibly helpful to the reader...
>> @@ -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_.
Thanks for the feedback!
-NGie
home |
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?347D22CE-6DDB-4AE9-879A-73743723B481>