From owner-svn-src-head@FreeBSD.ORG Wed Nov 12 15:13:44 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 7B92FC36; Wed, 12 Nov 2014 15:13:44 +0000 (UTC) Received: from smtp.des.no (smtp.des.no [194.63.250.102]) by mx1.freebsd.org (Postfix) with ESMTP id 22C1BDAC; Wed, 12 Nov 2014 15:13:43 +0000 (UTC) Received: from nine.des.no (smtp.des.no [194.63.250.102]) by smtp-int.des.no (Postfix) with ESMTP id 216E2A93E; Wed, 12 Nov 2014 15:13:43 +0000 (UTC) Received: by nine.des.no (Postfix, from userid 1001) id 87BA41795; Wed, 12 Nov 2014 16:13:42 +0100 (CET) From: =?utf-8?Q?Dag-Erling_Sm=C3=B8rgrav?= To: Bruce Evans Subject: Re: svn commit: r274340 - in head/sys: crypto/rijndael dev/random geom/bde References: <201411100944.sAA9icnN061962@svn.freebsd.org> <3C962D07-3AAF-42EA-9D3E-D8F6D9A812B0@FreeBSD.org> <86sihq5a2v.fsf@nine.des.no> <20141111223756.F3519@besplex.bde.org> <86oasd6dad.fsf@nine.des.no> <20141112100207.Q1068@besplex.bde.org> Date: Wed, 12 Nov 2014 16:13:42 +0100 In-Reply-To: <20141112100207.Q1068@besplex.bde.org> (Bruce Evans's message of "Wed, 12 Nov 2014 10:51:48 +1100 (EST)") Message-ID: <86h9y44fkp.fsf@nine.des.no> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (berkeley-unix) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, "Bjoern A. Zeeb" , src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 12 Nov 2014 15:13:44 -0000 Bruce Evans writes: > On Tue, 11 Nov 2014, [utf-8] Dag-Erling Sm=C3=B8rgrav wrote: > >> Bruce Evans writes: >>> -Wcast-qual is not a very good warning option since the official way >>> to remove qualifiers in C is to cast them away. Casting them away is >>> better than using the __DECONST() abomination. The option exists >>> because it is too easy for sloppy code to cast away const without >>> really intending to or when casting away const is done intentionally >>> but is an error. >> >> I agree that __DECONST() is ugly (not least because it strips all >> qualifiers, not just const, so it should be DEQUAL()), > > It is not quite that broken. __DEQUALIFIER() strips all qualifiers, > but __DECONST() starts by casting to const void *; thus if the initial > type has a volatile qualifier, and -Wcast-qual is configured, and > -Wcast-qual is not broken, then you get a cast-qual warning for > attempting to strip volatile. > >> but the >> alternative is worse. In my experience, the majority of cases where a >> cast discards a qualifier are bugs, with struct iov being one of very >> few legitimate use cases. > > That is a (design) bug too. Yes, I hate struct iov, but what is the alternative? An anonymous union inside struct iov so we have a non-const pointer for readv() and a const pointer for writev()? > The next level of design errors that require the cast is for the str*() > family. E.g., strchr() takes a 'const char *' and returns a plain > 'char *'. This is like the error for readv(), except strchr() is > lower level than readv(). This is trivially fixable by defining it as a macro instead. However, there is probably code out there that uses &strchr for some purpose or other, and any autoconf script that tests for the existence of strchr will break unless it uses AC_CHECK_DECL instead of AC_CHECK_FUNC (which is non-idiomatic but not wrong, as AC_CHECK_DECL checks for both). > The level below that is design errors errors in the C type system. > 'const' doesn't work right after the first level of indirection, > so it is impossible to declare functions like strtol() and excecv() > with the correct number of const's, and callers of these functions > need hacks to be comitbly broken. Tell me about it. It's a constant annoyance in PAM: int pam_get_item(const pam_handle_t *, int, const void **); > I certainly complain about their warning about "missing" parentheses for > && vs || and & vs |. This is like complaining about "missing" parentheses > for * vs +. These warnings are there for the same reason: frequent mistakes in both reading and writing complex boolean expressions. > All of these have a standard conventional precdedence and no > design errors for this in C (the C design errors for precedence are only > nearby, with & and | having much lower precedence than * and +, so much > lower that they are below comparison operators; That never fails to piss me off. 90% of the time I check operator(7) is to verify that I got & and | right. IMHO, foo & bar =3D=3D 0 should mean (foo & bar) =3D=3D 0, not foo & (bar =3D=3D 0) - although there are a few c= ases where the latter is useful: foo & bar is equivalent to foo || bar but without shortcut evaluation. I sometimes use this construct in unit tests. > > Apple's "goto fail" certificate verification bug was caused by code that > > was perfectly legal and looked fine at first glance but would have been > > caught by -Wunreachable-code. Unfortunately, turning it on in our tree > > breaks the build in non-trivially-fixable ways because it is triggered > > by const propagation into inline functions. > Turning on warnings finds too many problems in dusty decks. Compilers > shouldn't put new warnings under only warning flags since this mainly > finds non-bugs in code that is not dusty enough to need compiling with > no warning flags. -Wunreachable code is fine here since it is new. This particular case is not a "dusty deck". Try something like this - off the top of my head and somewhat contrived, but I think it is an adequate demonstration of the problem: /* * Return the (lexically) lesser of two strings. If one of * the arguments is NULL, return the other. */ static inline char * strmin(char *s1, char *s2) { =20=20=20=20=20=20=20=20 /* a */ if (s1 =3D=3D NULL) return (s2); /* b */ if (s2 =3D=3D NULL) return (s1); /* c */ return (strcmp(s1, s2) <=3D 0 ? s1 : s2); } Wherever you use strmin(), if gcc is able to determine that either s1 or s2 is NULL, you will get an "unreachable code" warning at point b or c, and possibly a bonus "condition is always true" warning. This is what happens when I try a gcc buildworld with -Wunreachable-code added at WARNS >=3D 3: cc1: warnings being treated as errors /home/des/freebsd/base/head/lib/libthr/thread/thr_affinity.c: In function '= _pthr ead_setaffinity_np': /home/des/freebsd/base/head/lib/libthr/thread/thr_umtx.h:139: warning: will= never be executed because _thr_umutex_unlock2() is called with defer =3D=3D NULL at that point. This seems to have been fixed somewhere between 4.2 and 4.8. Clang does not complain, but I don't know whether that is because it's smarter than GCC 4.2 or because -Wunreachable-code is unimplemented. There is no documentation of Clang's -W options. DES --=20 Dag-Erling Sm=C3=B8rgrav - des@des.no