Date: Wed, 12 Nov 2014 16:13:42 +0100 From: =?utf-8?Q?Dag-Erling_Sm=C3=B8rgrav?= <des@des.no> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, "Bjoern A. Zeeb" <bz@freebsd.org>, src-committers@freebsd.org Subject: Re: svn commit: r274340 - in head/sys: crypto/rijndael dev/random geom/bde Message-ID: <86h9y44fkp.fsf@nine.des.no> In-Reply-To: <20141112100207.Q1068@besplex.bde.org> (Bruce Evans's message of "Wed, 12 Nov 2014 10:51:48 %2B1100 (EST)") 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>
next in thread | previous in thread | raw e-mail | index | archive | help
Bruce Evans <brde@optusnet.com.au> writes: > On Tue, 11 Nov 2014, [utf-8] Dag-Erling Sm=C3=B8rgrav wrote: > >> Bruce Evans <brde@optusnet.com.au> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?86h9y44fkp.fsf>