[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

[David Blaikie]

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.

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.

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