Hi,<br><br><div class="gmail_quote">On Fri, Jun 1, 2012 at 10:41 PM, 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,<br>
<div><br>
On Fri, Jun 1, 2012 at 3:28 AM, Alexander Kornienko <<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>> wrote:<br>
> On Fri, Jun 1, 2012 at 2:11 AM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
</div><div><div>>> > This patch adds a "soft opt-in" option for<br>
>> > -Wimplicit-fallthroughdiagnostics. The reason for it is to provide a<br>
>> > 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<br>
>> > be<br>
>> > used for large code bases during a transition phase prior to turning on<br>
>> > -Wimplicit-fallthrough.<br>
>><br>
>> 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>
><br>
> Exactly. This warning is meant to be used during a transition period to<br>
> allow replacing comment annotations with compiler-checked annotations<br>
> method-by-method. I ran this diagnostic on a large code base, but I've not<br>
> found a switch with both intended fall-through and an unintended one. So<br>
> this option is not likely to detect any regressions of kind (1). But it<br>
> should help to prepare code base to use -Wimplicit-fallthrough.<br>
><br>
>><br>
>> In the patch...<br>
>><br>
>> --- tools/clang/include/clang/Basic/DiagnosticSemaKinds.td      (revision<br>
>> 157388)<br>
>> +++ tools/clang/include/clang/Basic/DiagnosticSemaKinds.td      (working<br>
>> 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<br>
>> method">,<br>
>> +  InGroup<ImplicitFallthroughPerMethod>, DefaultIgnore;<br>
>><br>
>> This new diagnostic is never actually emitted.<br>
><br>
> You're right. I missed this when changed from 'per-switch' approach to<br>
> 'per-method' one. Fixed.<br>
><br>
>><br>
>> 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>
><br>
> Done.<br>
><br>
> Please review the updated patch. Thanks!<br>
<br>
</div></div>Thanks! It looks like we now won't produce any warnings if the user<br>
specifies -Wimplicit-fallthrough -Wno-implicit-fallthrough-per-method.<br>
I'm not sure many people will want that, but I think it should still<br>
produce some implicit fallthrough warnings in that case.<br></blockquote><div>Thank you for review! </div></div>I didn't even think about this kind of perversion. I had to clarify for myself how subgroups of diagnostics are handled. Now "<font face="courier new, monospace">-Wimplicit-fallthrough -Wimplicit-fallthrough-per-method</font>" and "<font face="courier new, monospace">-Wimplicit-fallthrough -Wno-implicit-fallthrough-per-method</font>" work as "<font face="courier new, monospace">-Wimplicit-fallthrough</font>".<br>
<div><br>-- 
<div>Regards,</div><div>Alex</div></div>