[cfe-commits] [PATCH] "Soft opt-in" option for -Wimplicit-fallthrough

Richard Smith richard at metafoo.co.uk
Thu May 31 17:11:01 PDT 2012


Hi Alex,

> This patch adds a "soft opt-in" option for
> -Wimplicit-fallthroughdiagnostics. The reason for it is to provide a
> way to start using
> fall-through annotations without breaking (when treating warnings as
> errors) all code with unannotated fall-throughs. So it's only meant to be
> used for large code bases during a transition phase prior to turning on
> -Wimplicit-fallthrough.

It isn't clear to me whether this provides a lot of value. Presumably
the goal is to prevent code from regressing when it has already been
annotated with these attributes. I'm imagining two kinds of
regressions:

  (1) new fallthroughs being introduced, accidentally or deliberately
  (2) existing fallthrough annotations being placed or moved to
somewhere inappropriate

While this prevents the second kind of regression entirely, I think
the first kind is likely to be a much larger problem, and that is only
addressed in functions which already contain annotations. I would
expect only a small proportion of switch statements to contain
annotations, and no significant correlation between switch statements
containing annotations and regressions, so I don't think this will
prevent many regressions of kind (1).

In the patch...

--- tools/clang/include/clang/Basic/DiagnosticSemaKinds.td	(revision 157388)
+++ tools/clang/include/clang/Basic/DiagnosticSemaKinds.td	(working copy)
@@ -5281,6 +5281,9 @@
 def warn_unannotated_fallthrough : Warning<
   "unannotated fall-through between switch labels">,
   InGroup<ImplicitFallthrough>, DefaultIgnore;
+def warn_unannotated_fallthrough_per_method : Warning<
+  "unannotated fall-through between switch labels in partly annotated method">,
+  InGroup<ImplicitFallthroughPerMethod>, DefaultIgnore;

This new diagnostic is never actually emitted.

--- tools/clang/lib/Sema/AnalysisBasedWarnings.cpp	(revision 157388)
+++ tools/clang/lib/Sema/AnalysisBasedWarnings.cpp	(working copy)
@@ -652,13 +652,17 @@
   };
 }

-static void DiagnoseSwitchLabelsFallthrough(Sema &S, AnalysisDeclContext &AC) {
+static void DiagnoseSwitchLabelsFallthrough(Sema &S, AnalysisDeclContext &AC,
+                                            bool PerMethod) {
   FallthroughMapper FM(S);
   FM.TraverseStmt(AC.getBody());

   if (!FM.foundSwitchStatements())
     return;

+  if (PerMethod && FM.getFallthroughStmts().empty())
+    return;
+
   CFG *Cfg = AC.getCFG();

   if (!Cfg)
@@ -1154,7 +1158,10 @@

   if (Diags.getDiagnosticLevel(diag::warn_unannotated_fallthrough,
                               D->getLocStart()) !=
DiagnosticsEngine::Ignored) {
-    DiagnoseSwitchLabelsFallthrough(S, AC);
+    bool PerMethod = Diags.getDiagnosticLevel(
+                             diag::warn_unannotated_fallthrough_per_method,
+                             D->getLocStart()) != DiagnosticsEngine::Ignored;
+    DiagnoseSwitchLabelsFallthrough(S, AC, PerMethod);

It seems very strange to me that enabling the per-method diagnostic
*hides* diagnostics compared to the 'full' implicit fallthrough
diagnostic. I would suggest that, instead, the 'per-method' group is
made part of the implicit fallthrough group, not vice versa, and you
choose which diagnostic to produce based on whether there are any
fallthrough annotations in the method.



More information about the cfe-commits mailing list