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>
next in thread | previous in thread | raw e-mail | index | archive | help
> On Jan 17, 2016, at 21:13, Bruce Evans <brde@optusnet.com.au> wrote:
>=20
> On Mon, 18 Jan 2016, Garrett Cooper wrote:
>=20
>> Log:
>> Checkpoint work to bump WARNS to 6
>>=20
>> Try to fix -Wcast-align issues by using bcopy to assign the values in
>> the structure.
>=20
> 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.
>=20
> 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.
>=20
> 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 =E2=80=9Cdeeper copying=E2=80=9D in the structures to ensure =
that all of the fields are properly copied over in the bcopy, but I=E2=80=99=
ll look at what gdb says first.
>> =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D
>> --- =
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;
>=20
> The changes fix many style bugs (pointer variables not spelled with
> a p) but leave many others (verbose variable names).
Agreed. I=E2=80=99ll 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;
>=20
> '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=E2=80=99.
I agree in these cases. Unless it=E2=80=99s standard variable naming =
convention for C (rc/retval/rv being shorthand for =E2=80=9Creturn_value=E2=
=80=9D, etc), I try to stick with more verbose names to make the meaning =
of variables unambiguous. I agree though=E2=80=A6 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;
>=20
> Names related to bintimes are generally worse than for timevals, =
starting
> in the implementation not having prefixes in the struct member names.
>=20
> 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=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?347D22CE-6DDB-4AE9-879A-73743723B481>