Bruce Evans <b...@optusnet.com.au> writes:
> On Tue, 11 Nov 2014, [utf-8] Dag-Erling Smørgrav wrote:
>
>> Bruce Evans <b...@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 == 0 should mean
(foo & bar) == 0, not foo & (bar == 0) - although there are a few cases
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)
{
/* a */ if (s1 == NULL)
return (s2);
/* b */ if (s2 == NULL)
return (s1);
/* c */ return (strcmp(s1, s2) <= 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 >= 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 == 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
--
Dag-Erling Smørgrav - d...@des.no
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
