Hi Richard, On Fri, Jun 1, 2012 at 2:11 AM, Richard Smith <rich...@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
implicit-fallthrough-per-method.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
