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

[Alexander Kornienko]

Hi Richard,

On Fri, Jun 1, 2012 at 2:11 AM, Richard Smith <richard at metafoo.co.uk> wrote:

> Hi Alex,
>
> > 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!

-- 
Best regards,
Alexander Kornienko
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120601/703b7369/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: implicit-fallthrough-per-method.patch
Type: application/octet-stream
Size: 3319 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120601/703b7369/attachment.obj>