[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 2:55 PM, Joerg Sonnenberger
<joerg at britannica.bec.de> wrote:
> On Wed, Feb 08, 2012 at 09:50:22AM -0800, Ted Kremenek wrote:
>> On Feb 7, 2012, at 10:50 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> > I think it's a real pity to separate the two warnings, but I can
>> > understand the limitation/problem. Having this flag under /no/ group
>> > at all does leave me a bit concerned that it'll never be found/used,
>> > though.
>>
>> The rollout of new warnings is largely political.  Believe it or not,
>> many users have complained to use when we roll out new warnings under
>> -Wall.  Such users expect -Wall to have consistent behavior between releases.
>
> 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)

(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

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

- David

> My problem is that a lot of code uses default: branches either for
> proper diagnostics in debugging situations or for making sure that bad
> values trigger determistic failure cases.
>
> This warning is nice and good for an "isolated" code base like LLVM,
> where malicious input is not expected and not more important than any
> other bug. It doesn't work too well in central libraries or a kernel,
> where defensive programming is essential. Indicently, a switch statement
> that covers all enum values and a default branch is the typical way to
> check the validity of the interface. Unless there is a clear way to mark
> the default statement in this case as "invalid error case", I don't
> think it is a good idea to make this part of -Wall. Of course, having a
> proper way to do that would be a good idea anyway.
>
> Joerg
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits