Hi Richard,<div><br><div class="gmail_quote">On Fri, Jun 1, 2012 at 2:11 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Alex,<br>
<div class="im"><br>
> This patch adds a "soft opt-in" option for<br>
</div>> -Wimplicit-fallthroughdiagnostics. The reason for it is to provide a<br>
<div class="im">> way to start using<br>
> fall-through annotations without breaking (when treating warnings as<br>
> errors) all code with unannotated fall-throughs. So it's only meant to be<br>
> used for large code bases during a transition phase prior to turning on<br>
> -Wimplicit-fallthrough.<br>
<br>
</div>It isn't clear to me whether this provides a lot of value. Presumably<br>
the goal is to prevent code from regressing when it has already been<br>
annotated with these attributes. I'm imagining two kinds of<br>
regressions:<br>
<br>
  (1) new fallthroughs being introduced, accidentally or deliberately<br>
  (2) existing fallthrough annotations being placed or moved to<br>
somewhere inappropriate<br>
<br>
While this prevents the second kind of regression entirely, I think<br>
the first kind is likely to be a much larger problem, and that is only<br>
addressed in functions which already contain annotations. I would<br>
expect only a small proportion of switch statements to contain<br>
annotations, and no significant correlation between switch statements<br>
containing annotations and regressions, so I don't think this will<br>
prevent many regressions of kind (1).<br></blockquote><div>Exactly. This warning is meant to be used during a transition period to allow replacing comment annotations with compiler-checked annotations method-by-method. I ran this diagnostic on a large code base, but I've not found a switch with both intended fall-through and an unintended one. So this option is not likely to detect any regressions of kind (1). But it should help to prepare code base to use -Wimplicit-fallthrough.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
In the patch...<br>
<br>
--- tools/clang/include/clang/Basic/DiagnosticSemaKinds.td      (revision 157388)<br>
+++ tools/clang/include/clang/Basic/DiagnosticSemaKinds.td      (working copy)<br>
@@ -5281,6 +5281,9 @@<br>
 def warn_unannotated_fallthrough : Warning<<br>
   "unannotated fall-through between switch labels">,<br>
   InGroup<ImplicitFallthrough>, DefaultIgnore;<br>
+def warn_unannotated_fallthrough_per_method : Warning<<br>
+  "unannotated fall-through between switch labels in partly annotated method">,<br>
+  InGroup<ImplicitFallthroughPerMethod>, DefaultIgnore;<br>
<br>
This new diagnostic is never actually emitted.<br></blockquote><div>You're right. I missed this when changed from 'per-switch' approach to 'per-method' one. Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

It seems very strange to me that enabling the per-method diagnostic<br>
*hides* diagnostics compared to the 'full' implicit fallthrough<br>
diagnostic. I would suggest that, instead, the 'per-method' group is<br>
made part of the implicit fallthrough group, not vice versa, and you<br>
choose which diagnostic to produce based on whether there are any<br>
fallthrough annotations in the method.<br>
</blockquote></div>Done.<br><br>Please review the updated patch. Thanks!<br><br clear="all"><div>-- </div><div>Best regards,</div><div>Alexander Kornienko</div>
</div>