On Feb 7, 2012, at 10:50 PM, David Blaikie <[email protected]> wrote:
> On Tue, Feb 7, 2012 at 9:08 PM, Ted Kremenek <[email protected]> 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 >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
