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

[Richard Smith]

On Fri, Jun 1, 2012 at 3:51 PM, Alexander Kornienko <alexfh at google.com> wrote:
> On Fri, Jun 1, 2012 at 10:41 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
>> 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.
>
> Thank you for review!
> I didn't even think about this kind of perversion. I had to clarify for
> myself how subgroups of diagnostics are handled. Now "-Wimplicit-fallthrough
> -Wimplicit-fallthrough-per-method" and "-Wimplicit-fallthrough
> -Wno-implicit-fallthrough-per-method" work as "-Wimplicit-fallthrough".

Thanks! Patch LGTM. I'm still on the fence as to whether we want this
feature, but I think we'll only gain experience as to whether this
half-way position is valuable by trying it.