[PATCH] Warn on empty switch statements

[Reid Kleckner]

Should this have a -W flag?  Should it be on or off by default?

If we're not sure about how valuable this warning is, then we need a flag
to turn it off.  Then, there is a maintenance cost associated with adding
more -W flags.

The kinds of time wasters I used to hit are probably covered by
-Wconversion and -Wsign-compare, but I never turned them on.

If people agree that there is value to having a mode that tries (best
effort, not guaranteed) to warn when MSVC would, what about adding a flag
that groups these together?  Like -Wmsvc-[123].  Then if we implemented a
cl.exe compatible driver, we'd just alias /W3 to it.  And people could use
clang without breaking 'cl /W[123] /Wx' builds.

Then this switch warning and others like it could go under -Wmsvc-3,
without leading to a proliferation of -Wreally-obscure-warning flags.

I don't think you'd want to take this as far as maintaining a mapping of
numbered MSVC warnings to clang warnings.  If someone is using 'cl.exe /W3
/wd...' they can come up with an equivalent expression in clang flags.


On Mon, Jun 3, 2013 at 11: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?
>
> 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
>
>
> 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/1ba1cfbe/attachment.html>