[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 1:19 PM, Nico Weber <thakis at chromium.org> wrote:
> On Wed, Feb 8, 2012 at 12:24 PM, Ted Kremenek <kremenek at apple.com> wrote:
>> Out of curiosity, how did this trigger for you?  Would you object to this being under -Wall?
>
> It fired all over the place in the chromium codebase and its
> dependencies.

[*flogs dead horse in no particular direction* No doubt it did - but I
suppose my point is that so would -Wswitch or -Wparentheses on any
codebase that hadn't always been compiling with those flags to begin
with - they're stylistic devices designed to communicate intent to the
compiler (& other users) that isn't evident in the code as it would've
been written 'naturally'. The only reason code conforms to them is
because it's been woven around the lattice as it grew. Somehow,
someone decided that turning that on was worthwhile & putting it in
-Wall would encourage a good practice. I don't think it's impossible
that we could decide the same thing for future warnings the same way
these came to be in the first place.

But I do totally understand where Ted's coming from & realize that
users somehow think the current -Wall behavior is blessed without
understanding that to get there it would've caused pain to someone
just like them at some point in the past. Short of having a -Wall29,
-Wall30, -Wall31, etc and users turned on whichever one was available
when they started a new project, I'm not sure how we can support such
users exactly. We somehow have these 'blessed' stylistic warnings
(parentheses, switch, etc) now - and we could say they're abberations
& mistakes not to be repeated.

Chandler's thought of having more clearly documented semantics for
different groupings seems helpful - "this bucket of warnings represent
stylistic modifications that may have initial high false positive but
will constrain your code so as to communicate something to other
readers and the compiler & provide useful value over time" (&
importantly: new things with similar characteristics will be added
here over time - you can disable those as they come up, or spend the
time to address them each time you pickup a new compiler

*tires of flogging & goes back to writing code*]

> I just turned it off, so I can live with it being in
> -Wall. But one thing I really like about clang is its sense for good
> defaults in warnings. This warning feels like it's fairly noisy on
> existing code, and fixing it would be done mostly to appease the
> warning – in my experience, enum-switching code usually isn't terribly
> buggy. (I've been meaning to look at the recent switch changes and
> warnings and send a mail about them, but I didn't get to that yet. I
> feel like moving this warning out of -Wall and moving -Wswitch from
> -Wmost to -Wall is the right thing to do, but I haven't looked to
> closely yet.)

Hopefully our IRC discussion helped clarify what was going on in your
build - r150230 moves -Wswitch from -Wmost to -Wall as our
discussions. Hopefully that's OK with everyone else too.



>
> Maybe that's just change aversion, though :-)
>
> Nico
>
>>
>> On Feb 7, 2012, at 9:18 PM, Nico Weber wrote:
>>
>>> +1, thanks for this!
>>>
>>> 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.
>>>>
>>>> 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.  This warnings is fairly pedantic, which is good,
>>>> but not for everyone.  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