Hi, On Fri, Jun 1, 2012 at 10:41 PM, Richard Smith <rich...@metafoo.co.uk>wrote:
> Hi, > > On Fri, Jun 1, 2012 at 3:28 AM, Alexander Kornienko <ale...@google.com> > wrote: > > On Fri, Jun 1, 2012 at 2:11 AM, Richard Smith <rich...@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
implicit-fallthrough-per-method3.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
