[PATCH] Warn on empty switch statements

[Nico Weber]

On Mon, Jun 3, 2013 at 9:30 AM, Reid Kleckner <rnk at google.com> wrote:

> Should this have a -W flag?
>

Every warning should have some -W flag. There's a test that checks for
that. (test/Misc/warning-flags.c)


>  Should it be on or off by default?
>

The guideline so far was "if it's not useful enough to be on by default, it
probably shouldn't be in clang".


> 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.
>

I don't think adding warnings for things that aren't useful to warn about
is a good idea. clang's -Wuninitialized is more intelligent than gcc's too
for example, and adding a switch to dumb that down to gcc levels for
warning compatibility seems strange (which is what you're suggesting, but
for gcc instead of msvc).

The warning proposed in this patch doesn't seem like it'd have false
positives. Maybe someone could try running it on some largeish code base
(Firefox, Chromium, …) and see what it finds? I'm guessing 0 issues and 0
false positives, which is probably good enough to allow it since it has
some value.


>
> 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.
>>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130603/c7f8a67a/attachment.html>