[cfe-commits] r150055 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td test/Sema/switch.c

[Joerg Sonnenberger]

On Wed, Feb 08, 2012 at 03:04:44PM -0800, David Blaikie wrote:
> > The critical question is whether (a) a warning finds a useful number of
> > real bugs (b) doesn't have too many false positives and (c) has a
> > reasonable workaround for the cases of (b).
> 
> (a) is hard to say because this warning itself wasn't really intended
> to find current bugs - it was intended to ensure you find future bugs
> (when you add an enum value & then fail to update a switch)

Experience with large projects say that all likely bugs have been made
in the past already :)

> (b) hard to say - like -Wparentheses and the original -Wswitch
> warning, this is more a convention designed to communicate extra
> information to the compiler to check semantics that aren't explicit in
> the code

I agree that this is an edge case.

> (c) The most immediate is to simply cast the switch condition
> expression to an integer type. This seems like it communicates clearly
> to the compiler "this isn't actually bounded by the enum type - I
> expect it to be any possible integer value". Alternatively, to do the
> error checking before the switch (assert(x >= first_enum && x <=
> last_enum) - and whenever the -Wswitch enum triggers on the following
> switch, it'd be quite close/easy to update the assert too).

So what is the correct integer type to use for the cast? Keep in mind
it depends on the values of the enum members. As such, casting doesn't
sound like the correct answer. Checking the range also doesn't work in
most interesting cases, since enums aren't always consecutive either.

More importantly, both cases just destroys the analysis potential again.
Just because I want to program defensively, doesn't mean the check isn't
desirable. Using __builtin_unreachable() as marker works if there is no
sane error recovery. That is good enough for checks deep in the gusts of
code. It doesn't help for code on module borders. The optimisation
properties of __builtin_unreachable() may also be undesired. This seems
to call for a __builtin_invalid_case() :) Kind of like a semantically
stronger version of the branch prediction statements.

Joerg