Hi, On Fri, Jun 1, 2012 at 3:28 AM, Alexander Kornienko <[email protected]> wrote: > On Fri, Jun 1, 2012 at 2:11 AM, Richard Smith <[email protected]> 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. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
