On Fri, Jun 1, 2012 at 3:51 PM, Alexander Kornienko <[email protected]> wrote: > On Fri, Jun 1, 2012 at 10:41 PM, Richard Smith <[email protected]> > wrote: >> 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. > > 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".
Thanks! Patch LGTM. I'm still on the fence as to whether we want this feature, but I think we'll only gain experience as to whether this half-way position is valuable by trying it. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
