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

[Bob Wilson]

On Feb 7, 2012, at 10:50 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Tue, Feb 7, 2012 at 9:08 PM, Ted Kremenek <kremenek at apple.com> wrote:
>> Author: kremenek
>> Date: Tue Feb  7 23:08:58 2012
>> New Revision: 150055
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=150055&view=rev
>> Log:
>> Move -Wcovered-switch-default out of -Wswitch (and -Wall), and make it an opt-in warning.
> 
> 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.
> 
> As a side note (& I wouldn't've minded a more all-party discussion of
> this - though I know it came up once or twice on IRC) Clang's -Wswitch
> is under -Wmost (on by default) whereas GCC's -Wswitch is under -Wall
> (off by default). One of the possible solutions to this issue (of
> -Wcovered-switch-default) I'd considered was to fix this was to make
> Clang's -Wswitch behavior consistent with GCC's (thus turning it, and
> -Wcovered-switch-default, off by default). Yes, this would still put
> -Wcovered-switch-default would be under -Wall & thus, perhaps, still a
> bit noisy but it's more likely someone actually wants -Wswitch if they
> turn on -Wall & it'd be helpful if they got the other half of that
> warning (-Wcovered-switch-default) so -Wswitch was reliable.

We have found that a lot of real-world code builds with -Wall and triggers this warning.  Many Apple projects have zero-warning policies, and this new warning was incredibly troublesome.  I don't think Apple's code in unique in this, especially since some of the projects in question are open-source.

If you have ideas for how we could improve the discoverability of this, that would be great.

> 
>> This is a great warning, but it was observed that a ton of real world code violates
>> it all the time for (semi-)legitimate reasons.
> 
> Last time I brought up any off-by-default warnings Doug seemed to veto
> them. (I'm not sure if he's even seen this warning/has an opinion on
> it - I'm a bit concerned that he won't like it once he sees it)

Doug is away at a standards meeting.  He won't notice ;-)

> 
>>  This warnings is fairly pedantic, which is good,
>> but not for everyone.
> 
> Could we turn it on in the LLVM Makefiles (llvm/Makefile.rules ~ line
> 650 - I'm not sure if we have to do something else for the cmake
> build)? Do we have a way to turn on certain flags only in the presence
> of Clang?

I'm in favor of that if there's an easy way to do it.

> 
>>  For example, there is a fair amount of idiomatic code out there
>> that does "default: abort()", and similar idioms.
>> 
>> Addresses <rdar://problem/10814651>.
>> 
>> Modified:
>>    cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>>    cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>    cfe/trunk/test/Sema/switch.c
>> 
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=150055&r1=150054&r2=150055&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Tue Feb  7 23:08:58 2012
>> @@ -184,9 +184,9 @@
>>  def : DiagGroup<"strict-prototypes">;
>>  def StrictSelector : DiagGroup<"strict-selector-match">;
>>  def MethodDuplicate : DiagGroup<"duplicate-method-match">;
>> -def SwitchEnum     : DiagGroup<"switch-enum">;
>>  def CoveredSwitchDefault : DiagGroup<"covered-switch-default">;
>> -def Switch         : DiagGroup<"switch", [CoveredSwitchDefault]>;
>> +def SwitchEnum     : DiagGroup<"switch-enum">;
>> +def Switch         : DiagGroup<"switch">;
>>  def Trigraphs      : DiagGroup<"trigraphs">;
>> 
>>  def : DiagGroup<"type-limits">;
>> 
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=150055&r1=150054&r2=150055&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Feb  7 23:08:58 2012
>> @@ -5064,7 +5064,7 @@
>> 
>>  def warn_unreachable_default : Warning<
>>   "default label in switch which covers all enumeration values">,
>> -  InGroup<CoveredSwitchDefault>;
>> +  InGroup<CoveredSwitchDefault>, DefaultIgnore;
>>  def warn_not_in_enum : Warning<"case value not in enumerated type %0">,
>>   InGroup<Switch>;
>>  def err_typecheck_statement_requires_scalar : Error<
>> 
>> Modified: cfe/trunk/test/Sema/switch.c
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/switch.c?rev=150055&r1=150054&r2=150055&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/Sema/switch.c (original)
>> +++ cfe/trunk/test/Sema/switch.c Tue Feb  7 23:08:58 2012
>> @@ -1,4 +1,4 @@
>> -// RUN: %clang_cc1 -fsyntax-only -verify -Wswitch-enum %s
>> +// RUN: %clang_cc1 -fsyntax-only -verify -Wswitch-enum -Wcovered-switch-default %s
>>  void f (int z) {
>>   while (z) {
>>     default: z--;            // expected-error {{statement not in switch}}
>> 
>> 
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits