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

[Alexander Kornienko]

Hi,

On Fri, Jun 1, 2012 at 10:41 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> 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.
>
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".

--
Regards,
Alex
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120602/400fcb84/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: implicit-fallthrough-per-method3.patch
Type: application/octet-stream
Size: 5160 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120602/400fcb84/attachment.obj>