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.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
