[cfe-commits] r148640 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaStmt.cpp test/Sema/switch.c test/Sema/warn-unreachable.c test/SemaCXX/gnu-case-ranges.cpp test/SemaCXX/return-n

[Fariborz Jahanian]

On Jan 21, 2012, at 11:16 AM, David Blaikie wrote:

> On Sat, Jan 21, 2012 at 11:02 AM, Fariborz Jahanian <fjahanian at apple.com> wrote:
>> 
>> On Jan 21, 2012, at 10:12 AM, David Blaikie wrote:
>> 
>>> Author: dblaikie
>>> Date: Sat Jan 21 12:12:07 2012
>>> New Revision: 148640
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=148640&view=rev
>>> Log:
>>> Add -Wswitch-enum-redundant-default.
>>> 
>>> This warning acts as the complement to the main -Wswitch-enum warning (which
>>> warns whenever a switch over enum without a default doesn't cover all values of
>>> the enum) & has been an an-doc coding convention in LLVM and Clang in my
>>> experience. The purpose is to ensure there's never a "dead" default in a
>>> switch-over-enum because this would hide future -Wswitch-enum errors.
>> 
>> We have a related problem with -Wswitch-enum in clang. with this option a default should not turn
>> off the warning when default covers un-used case labels. Currently clang's behavior of --Wswitch and
>> -Wswitch-enum  are the same. I think fixing that will not make the new option necessary.
>> 
>> -Wswitch
>>           Warn whenever a "switch" statement has an index of enumerated type and lacks a "case" for one or
>>           more of the named codes of that enumeration.  (The presence of a "default" label prevents this
>>           warning.)  "case" labels outside the enumeration range also provoke warnings when this option is
>>           used.  This warning is enabled by -Wall.
>> 
>> -Wswitch-enum
>>           Warn whenever a "switch" statement has an index of enumerated type and lacks a "case" for one or
>>           more of the named codes of that enumeration.  "case" labels outside the enumeration range also
>>           provoke warnings when this option is used.
>> 
>> 
>> int test18() {
>>  enum { A, B } a;
>>  switch (a) {
>>  case A: return 0;
>>  default: return 2;
>> }
>> }
> 
> I'm not entirely sure I follow you - you mean that -Wswitch-enum
> should warn whenever there's a switch-over-enum that doesn't
> explicitly list all the enum values, even in the presence of a
> default? That seems like it would be very noisy/less valuable. There's
> often cases where one wants to handle an explicit subset of enum
> values & ignore the rest (granted, this case does devalue the switch
> warning).
> 
> I was wondering what the difference between -Wswitch and -Wswitch-enum
> was, but when I read the GCC docs it wasn't clear to me.
> 
> Now that I search again, the docs I found are more explicit and
> support your statement:
> 
> -Wswitch
>    Warn whenever a switch statement has an index of enumerated type
> and lacks a case for one or more of the named codes of that
> enumeration. (The presence of a default label prevents this warning.)
> case labels outside the enumeration range also provoke warnings when
> this option is used (even if there is a default label). This warning
> is enabled by -Wall.
> 
> -Wswitch-enum
>    Warn whenever a switch statement has an index of enumerated type
> and lacks a case for one or more of the named codes of that
> enumeration. case labels outside the enumeration range also provoke
> warnings when this option is used. The only difference between
> -Wswitch and this option is that this option gives a warning about an
> omitted enumeration code even if there is a default label.

Yes, clang currently treats -Wswitch-enum same as -Wswitch. Maybe, while you
are at it you can address this too. Once -Wswitch-enum is corrected, a "default' can no
longer hide missing case labels.

> 
> http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
> 
> So this would seem to say that the new warning I added is applicable
> to both -Wswitch and -Wswitch-enum. It's probably less valuable in the
> latter case - since, if you're using -Wswitch-enum, you don't need to
> worry about the presence or absence of 'default' labels as much. They
> might be unreachable code but they won't hide future mistakes.

Yes.

- Thanks, Fariborz

> 
> So to correct my statement in the patch,
> -Wswitch-enum-redundant-default is important for -Wswitch for those
> reasons, but not important for -Wswitch-enum because the latter will
> catch you regardless of the default.
> 
> (excuse the rambling reply - I think I found the right conclusion, though ;))
> 
> - David