[cfe-commits] r150055 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td test/Sema/switch.c

[Chandler Carruth]

Sorry to come late to this thread, but I'm going to try and provide another
perspective here, specifically where Ted asked for other perspectives:

On Wed, Feb 8, 2012 at 11:02 AM, Ted Kremenek <kremenek at apple.com> wrote:

> A fair point.  I think it's worth pointing out that a fair number of
> people disliked this change, so much that they requested that we could put
> the inverse case under a different warning flag so they could turn it off.
>
> I'm leaning that this should be under -Wall after all, but it would be
> great to hear other opinions here.
>

 I think the problem with this warning under -Wall is this: there are
competing coding patterns.

With -Wparentheses, by luck, history, or whatever, we have gotten the
overwhelming majority of code using the same pattern: (x == y) and ((x =
y)). That means we get to warn very easily on both sides of this pattern.

With this warning we have gotten wonderful consistency of
switches-over-enums *without* default cases, but there remain many patterns
with defaults:

switch (x) {
  default:
  case X:
  ...
}

switch (x) {
  default: abort();
  ...
}

switch (x) {
  default: assert(0 && ...);
  ...
}


While we have some good evidence that there is a better pattern (the one
this warning pushes people toward), but there is a considerable amount of
code that currently follows one of the old patterns.

To me, the question of whether we should try to drive code toward a single
style through a -Wall should be answered by looking at how buggy the
existing patterns are, and how frequently they are followed. For code I've
looked at, this warning occurs fairly frequently, and has found few or no
bugs. For code I've looked at, the converse pattern added to -Wparentheses
fired fairly infrequently, and in a reasonable number of cases found bugs.

Given that, I think -Wcovered-switch-default is useful to folks who are
willing to switch coding patterns to one we can defend against future bugs,
but I fear it has too high of a "falso positive" from people who are
satisfied with finding related bugs via 'abort()' or 'assert()' or whatever
to place under -Wall.

I think we should have two categories here, with better names:

-Wimportant -- These are warnings we expect to be valuable even for large
existing codebases. We recommend essentially everyone that can use these.

-Wuseful -- These are warnings which have high utility, but may require
non-trivial code changes to accomodate.

There are probably still better names, these are what I came up with off
the cuff.

Currently we use "-Wall" to mean the first of these. I wonder if we could
use -Wextra to mean the second. I also wonder whether we should add aliases
with more clear names.

-Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120208/63648cb9/attachment.html>