[PATCH] Warn on empty switch statements

[Richard Smith]

On Mon, Jun 3, 2013 at 8:23 AM, Aaron Ballman <aaron at aaronballman.com>wrote:

> Ping?
>
> I've given this some more thought, and I feel like Reid and Daniel (in
> the PR) hit the nail on the head: it is really nice to be able to
> write code in clang and not have to worry that I've introduced a
> warning in MSVC, especially since I try to compile everything with
> warnings as errors.  I understand the fear of trying to match
> diagnostics with other compilers, but where's the harm if it's not
> overly chatty and it can catch bugs?
>

The harm is the increased maintenance burden of extra warnings. Conversely,
where's the benefit? I think there are three options here:

1) This does not occur in real-world code => the warning has no value, not
even for MS compatibility
2) This occurs in real-world code and it's usually not a bug => the warning
has questionable value and probably shouldn't be in Clang
3) This occurs in real-world code and it's usually a bug => the warning has
value, and it should be possible to provide real examples of bugs it finds

There's clearly grey areas here, but in essence, if this warning has value,
it should be possible to provide examples of real code which triggers the
warning.


> Also, I think the warning adds value over the empty statement warning
> because the situations are different.  Eg)
>
> switch (expr);  // Should warn because the ; is suspicious
> {
>   /* stuff */
> }
>
> // vs
>
> switch (expr) {
>   /* ToDo: finish this */
> }  // Should warn because the emptiness of the block is suspicious
>

If I wrote the above code, I think I would not want a warning, any more
than I'd want a "comment contains FIXME" warning.

Here's one conceivable false positive:

template<typename T> void f(T t) {
  switch (t) {} // produce an early diagnostic if we can't switch on 'T',
this template relies on that.
  do_complex_deeply_nested_stuff(t);
}

("switch (...) {}" is one of the ways we tell people to suppress the
warning which is produced for "switch (...);").

Also, I can imagine this appearing in generated code or code using xmacros.
I would want the warning suppressed at least for a switch on an empty enum.

In the case where the user really does want an empty switch statement,
> they can selectively turn the diagnostic off.  This really shouldn't
> be a common case, after all.
>
> ~Aaron
>
> On Wed, May 29, 2013 at 4:10 PM, Reid Kleckner <rnk at google.com> wrote:
> > On Wed, May 29, 2013 at 3:49 PM, Richard Smith <richard at metafoo.co.uk>
> > wrote:
> >>
> >> On Wed, May 29, 2013 at 7:35 AM, Aaron Ballman <aaron at aaronballman.com>
> >> wrote:
> >>>
> >>> -Wunreachable-code doesn't warn on empty statement bodies for the
> >>> switch.  So, for instance:
> >>>
> >>> int main() {
> >>>   int i = 0;
> >>>   switch(i) {}
> >>> }
> >>>
> >>> yields no warnings in clang with -Wunreachable-code (or -Wall), but at
> >>> /W3 in MSVC you get: warning C4060: switch statement contains no
> >>> 'case' or 'default' labels.  Will it catch lots of bugs?  Probably
> >>> not.  But it also should have basically no false positives either.  It
> >>> would be nice for people who want their code to also be warning-free
> >>> under MSVC.
> >>
> >>
> >> I think we need more justification for adding the warning than this.
> >>
> >> It doesn't seem reasonable to expect us to implement the union of all
> >> warnings which every existent compiler produces. Some of those warnings
> are
> >> really dumb. For instance, /W3 also enables C4800, which I don't think
> we
> >> will ever want to implement.
> >
> >
> > I agree that warning (forcing value to bool) is really, really bad, and I
> > hope most serious projects turn it off.
> >
> > However, I can say it's pretty annoying if you're working on
> cross-platform
> > project and have to push to another platform to build just to get some
> > compiler warnings for a compiler which you're expected to keep warning
> > clean, value judgements about its diagnostic quality aside.  That cycle
> is
> > long and painful, and if you skip it people often yell at you.
> >
> > While clang will never be able to paper over the differences between
> > platforms, having some off-by-default diagnostics like this could save
> users
> > some time and increase the likelihood that their changes Just Work on the
> > first push.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130603/e6c9c5b3/attachment.html>