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

[Richard Smith]

Hi,

On Fri, Jun 1, 2012 at 3:28 AM, Alexander Kornienko <alexfh at google.com> wrote:
> On Fri, Jun 1, 2012 at 2:11 AM, Richard Smith <richard at metafoo.co.uk> wrote:
>> > 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).
>
> 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.
>
>>
>> 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.
>
> You're right. I missed this when changed from 'per-switch' approach to
> 'per-method' one. Fixed.
>
>>
>> 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.
>
> Done.
>
> Please review the updated patch. Thanks!

Thanks! It looks like we now won't produce any warnings if the user
specifies -Wimplicit-fallthrough -Wno-implicit-fallthrough-per-method.
I'm not sure many people will want that, but I think it should still
produce some implicit fallthrough warnings in that case.