[Diagnostics] Implemented support for -Wswitch-default

[xbolva00]

Removed redundant line in DiagnosticGroups

[riccibruno]

Is it intentional to warn even if all the cases are covered ?

[xbolva00]

GCC warns even for that case, so this patch implements GCC's behaviour for now.

[xbolva00]

include/clang/Basic/DiagnosticIDs.h

39

@rsmith maybe this should be a separate patch? I hit this limit in this patch.

[riccibruno]

include/clang/Basic/DiagnosticIDs.h

39

Is there any downside to just bumping it to, say, 4000 ?

[xbolva00]

include/clang/Basic/DiagnosticIDs.h

39

I think it is fine to bump it more..

[aaron.ballman]

Do we have evidence of false positive vs true positive rates for this catching actual problems in real world code? I realize that this is for GCC compatibility, but because it's default-off, I'm wondering where the utilities lies.

include/clang/Basic/DiagnosticIDs.h

39

I think it should be done in a separate patch, and I agree that we should bump to 4000 for right now to give us some wiggle room.

[xbolva00]

Some coding guidelines may require switch to have always default label. Even if devs know that default is not reachable, they can add default: abort(); or assert to increase safety (and warning will be silenced).

Yes, it not suitable to be enabled by default, but I still think it is good to have it.

[aaron.ballman]

In D61288#1486006, @xbolva00 wrote:

Some coding guidelines may require switch to have always default label. Even if devs know that default is not reachable, they can add default: abort(); or assert to increase safety (and warning will be silenced).

Yes, it not suitable to be enabled by default, but I still think it is good to have it.

We typically don't add new, default-off warnings because experience has shown that users don't enable them. The coding guidelines argument is somewhat persuasive, but I wonder whether this is better handled through clang-tidy checks rather than the compiler itself -- that's where we put other diagnostics that may not be suitable for the compiler. Have you thought about surfacing this functionality that way?

[lebedev.ri]

In D61288#1486008, @aaron.ballman wrote:

In D61288#1486006, @xbolva00 wrote:

Some coding guidelines may require switch to have always default label. Even if devs know that default is not reachable, they can add default: abort(); or assert to increase safety (and warning will be silenced).

Yes, it not suitable to be enabled by default, but I still think it is good to have it.

We typically don't add new, default-off warnings because experience has shown that users don't enable them. The coding guidelines argument is somewhat persuasive, but I wonder whether this is better handled through clang-tidy checks rather than the compiler itself -- that's where we put other diagnostics that may not be suitable for the compiler. Have you thought about surfacing this functionality that way?

I honestly have to agree. I think it would be best as a clang-tidy check, because i can fully envision the exact opposite guideline, "avoid default so you get notified about every switch that needs to be updated".

[xbolva00]

I am not familiar with clang-tidy’s codebase and I personally prefer good compiler warnings than dependency on another tool (clang-tidy). I mean the cases when diagnostic check is easy to do in the compiler.

And in semaexpr we have all we need and it is simple to do it.

But if majority of reviewers disagree with compiler warning, I will close this revision.

[aaron.ballman]

In D61288#1486034, @xbolva00 wrote:

I am not familiar with clang-tidy’s codebase and I personally prefer good compiler warnings than dependency on another tool (clang-tidy). I mean the cases when diagnostic check is easy to do in the compiler.

And in semaexpr we have all we need and it is simple to do it.

But if majority of reviewers disagree with compiler warning, I will close this revision.

While it is easy to do in the compiler frontend, I am not yet convinced it's really worth having there compared to clang-tidy. FWIW, I'd be happy seeing it in clang-tidy.

include/clang/Basic/DiagnosticGroups.td

def Shadow : DiagGroup<"shadow", [ShadowFieldInConstructorModified,
                                   ShadowIvar]>;
 def ShadowAll : DiagGroup<"shadow-all", [Shadow, ShadowFieldInConstructor,
                                          ShadowUncapturedLocal, ShadowField]>;
 
 def Shorten64To32 : DiagGroup<"shorten-64-to-32">;
 def : DiagGroup<"sign-promo">;
 def SignCompare : DiagGroup<"sign-compare">;
 def : DiagGroup<"stack-protector">;
-def : DiagGroup<"switch-default">;
 def : DiagGroup<"synth">;
 def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">;
 def SizeofArrayDecay : DiagGroup<"sizeof-array-decay">;
 def SizeofPointerMemaccess : DiagGroup<"sizeof-pointer-memaccess">;
 def MemsetTransposedArgs : DiagGroup<"memset-transposed-args">;
 def DynamicClassMemaccess : DiagGroup<"dynamic-class-memaccess">;
 def NonTrivialMemaccess : DiagGroup<"nontrivial-memaccess">;
 def SuspiciousBzero : DiagGroup<"suspicious-bzero">;

 
 def InvalidOffsetof : DiagGroup<"invalid-offsetof">;
 def : DiagGroup<"strict-prototypes">;
 def StrictSelector : DiagGroup<"strict-selector-match">;
 def MethodDuplicate : DiagGroup<"duplicate-method-match">;
 def ObjCCStringFormat : DiagGroup<"cstring-format-directive">;
 def CoveredSwitchDefault : DiagGroup<"covered-switch-default">;
 def SwitchBool     : DiagGroup<"switch-bool">;
+def SwitchDefault  : DiagGroup<"switch-default">;
 def SwitchEnum     : DiagGroup<"switch-enum">;
 def Switch         : DiagGroup<"switch">;
 def EnumCompareSwitch : DiagGroup<"enum-compare-switch">;
 def EnumCompare       : DiagGroup<"enum-compare", [EnumCompareSwitch]>;
 def ImplicitFallthroughPerFunction :
   DiagGroup<"implicit-fallthrough-per-function">;
 def ImplicitFallthrough  : DiagGroup<"implicit-fallthrough",
                                      [ImplicitFallthroughPerFunction]>;

include/clang/Basic/DiagnosticIDs.h

enum {
       DIAG_SIZE_DRIVER        =  200,
       DIAG_SIZE_FRONTEND      =  150,
       DIAG_SIZE_SERIALIZATION =  120,
       DIAG_SIZE_LEX           =  400,
       DIAG_SIZE_PARSE         =  500,
       DIAG_SIZE_AST           =  150,
       DIAG_SIZE_COMMENT       =  100,
       DIAG_SIZE_CROSSTU       =  100,
-      DIAG_SIZE_SEMA          = 3500,
+      DIAG_SIZE_SEMA          = 3600,
       DIAG_SIZE_ANALYSIS      =  100,
       DIAG_SIZE_REFACTORING   = 1000,
     };
     // Start position for diagnostics.
     enum {
       DIAG_START_COMMON        =                          0,
       DIAG_START_DRIVER        = DIAG_START_COMMON        + DIAG_SIZE_COMMON,
       DIAG_START_FRONTEND      = DIAG_START_DRIVER        + DIAG_SIZE_DRIVER,

include/clang/Basic/DiagnosticSemaKinds.td

def err_fallthrough_attr_invalid_placement : Error<
   "fallthrough annotation does not directly precede switch label">;
 def warn_fallthrough_attr_unreachable : Warning<
   "fallthrough annotation in unreachable code">,
   InGroup<ImplicitFallthrough>, DefaultIgnore;
 
 def warn_unreachable_default : Warning<
   "default label in switch which covers all enumeration values">,
   InGroup<CoveredSwitchDefault>, DefaultIgnore;
+def warn_missing_default_label : Warning<
+  "missing default label in switch">,
+  InGroup<SwitchDefault>, DefaultIgnore;
 def warn_not_in_enum : Warning<"case value not in enumerated type %0">,
   InGroup<Switch>;
 def warn_not_in_enum_assignment : Warning<"integer constant not in range "
   "of enumerated type %0">, InGroup<DiagGroup<"assign-enum">>, DefaultIgnore;
 def err_typecheck_statement_requires_scalar : Error<
   "statement requires expression of scalar type (%0 invalid)">;
 def err_typecheck_statement_requires_integer : Error<
   "statement requires expression of integer type (%0 invalid)">;

lib/Sema/SemaStmt.cpp

if (DefaultStmt *DS = dyn_cast<DefaultStmt>(SC)) {
           break;
         }
         CaseRanges.push_back(std::make_pair(LoVal, CS));
       } else
         CaseVals.push_back(std::make_pair(LoVal, CS));
     }
   }
 
+  if (!TheDefaultStmt)
+    Diag(SS->getSwitchLoc(), diag::warn_missing_default_label);
+
   if (!HasDependentValue) {
     // If we don't have a default statement, check whether the
     // condition is constant.
     llvm::APSInt ConstantCondValue;
     bool HasConstantCond = false;
     if (!HasDependentValue && !TheDefaultStmt) {
       Expr::EvalResult Result;
       HasConstantCond = CondExpr->EvaluateAsInt(Result, Context,

test/Sema/switch-default.c

+// RUN: %clang_cc1 -fsyntax-only -verify -Wswitch-default %s
+
+enum E { case1, case2 };
+
+void warn(int x, enum E e) {
+  switch (x) { // expected-warning {{missing default label in switch}}
+  case 0:
+    break;
+  }
+
+  switch (e) { // expected-warning {{missing default label in switch}}
+  case case1:
+    break;
+  case case2:
+    break;
+  }
+}
+
+void no_warn(int x, enum E e) {
+  switch (x) {
+  case 0:
+    break;
+  default:
+    break;
+  }
+
+  switch (e) {
+  case case1:
+    break;
+  case case2:
+    break;
+  default:
+    break;
+  }
+}

test/SemaCXX/switch-default.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -Wswitch-default %s
+
+enum E { case1, case2 };
+
+void warn(int x, enum E e) {
+  switch (x) { // expected-warning {{missing default label in switch}}
+  case 0:
+    break;
+  }
+
+  switch (e) { // expected-warning {{missing default label in switch}}
+  case case1:
+    break;
+  case case2:
+    break;
+  }
+}
+
+void no_warn(int x, enum E e) {
+  switch (x) {
+  case 0:
+    break;
+  default:
+    break;
+  }
+
+  switch (e) {
+  case case1:
+    break;
+  case case2:
+    break;
+  default:
+    break;
+  }
+}