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

[David Blaikie]

On Wed, Feb 8, 2012 at 3:33 PM, Joerg Sonnenberger
<joerg at britannica.bec.de> wrote:
> 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 :)

Right - well, for whatever that's worth there were lots of real
violations of this rule in LLVM - obviously because we actually tried
to maintain this style/approach there weren't (m)any deliberate uses
of defaults on covered switches.

But yeah - more experience with other code bases is important, such as yours.

>> (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.

std::underlying_type<e>::type

> As such, casting doesn't
> sound like the correct answer.

Sort of seems like it is - if you actually want to tolerate the value
being outside the actual enum values that would communicate it clearly
to the compiler.

> Checking the range also doesn't work in
> most interesting cases, since enums aren't always consecutive either.

That's fair.

> 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.

Still makes it hard to tell whether it's deliberate - when you do add
a new enum value what does it mean? Should -Wswitch flag this, or is
it just assumed that the user intended that the new enum value would
never occur in this switch?

> That is good enough for checks deep in the gusts of
> code. It doesn't help for code on module borders.

If you don't have any control over the bounds/you expect it to have
other values I'm still sort of seeing the cast to integral type to
describe the situation you're in.  I suppose you lose things like the
ability to be warned about testing values that aren't any of the
enums, or -Wswitch (though these scenarios don't get this
functionality at the moment anyway), etc.

> This seems
> to call for a __builtin_invalid_case() :) Kind of like a semantically
> stronger version of the branch prediction statements.

I'm not sure it's worth coming to that... ;)

- David