[cfe-commits] r150055 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td test/Sema/switch.c
David Blaikie
dblaikie at gmail.com
Wed Feb 8 10:06:31 PST 2012
On Wed, Feb 8, 2012 at 8:38 AM, Bob Wilson <bob.wilson at apple.com> wrote:
>
> 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.
I could totally believe that (even LLVM did, of course - and we at
least had it as an ad-hoc code review policy) - & sorry I didn't
highlight this issue (especially the fact that the warning would be on
by default) more clearly during code review.
> Many Apple projects have zero-warning policies, and this new warning was incredibly troublesome.
Good policies, certainly - and I can see how any new warning would
produce a migration issue.
> I don't think Apple's code in unique in this, especially since some of the projects in question are open-source.
Yeah, I can totally believe that. Without the warning in place it's
very easy to violate it even when you're trying not to.
> If you have ideas for how we could improve the discoverability of this, that would be great.
I really don't know - and I think that's a recurring & significant
problem (at least going on past discussions with Doug about
off-by-default warnings being pretty much unused in his
opinion/experience) especially with new codebases. If we hold
ourselves to a bar of no false positives on existing code then every
new codebase that is using Clang from the start is no better off than
the worst legacy codebase we support.
Existing warnings in -Wall (like, say, '-Wparentheses') would be
completely unthinkable to add if they weren't there already, for the
same reason - yet we accept these as good (ish?) warnings that help
find bugs. Even -Wswitch itself would easily flag false positives if a
code base wasn't already consciously adhering to it.
>>> 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 ;-)
Yeah - though he's had a huge CR backlog for a while now so I've no
idea how long it'll be before he does at least glance at this (or
whether he does when it's signed off/checked in by another core
developer). I don't really want to sneak things in under the radar, so
to speak.
>>> 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.
Yeah - I was mostly wondering what the easy way to do it was ;) I'll
see if I can figure out how to do some clang-detection logic. Though
that may still break people using older builds of clang unless I can
do a really specific revision check.
- David
>>> 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
>
More information about the cfe-commits
mailing list