On Mon, Jun 3, 2013 at 8:23 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Ping?<br>
<br>
I've given this some more thought, and I feel like Reid and Daniel (in<br>
the PR) hit the nail on the head: it is really nice to be able to<br>
write code in clang and not have to worry that I've introduced a<br>
warning in MSVC, especially since I try to compile everything with<br>
warnings as errors. I understand the fear of trying to match<br>
diagnostics with other compilers, but where's the harm if it's not<br>
overly chatty and it can catch bugs?<br></blockquote><div><br></div><div>The harm is the increased maintenance burden of extra warnings. Conversely, where's the benefit? I think there are three options here:</div><div>
<br></div><div>1) This does not occur in real-world code => the warning has no value, not even for MS compatibility</div><div>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</div>
<div>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</div><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, I think the warning adds value over the empty statement warning<br>
because the situations are different. Eg)<br>
<br>
switch (expr); // Should warn because the ; is suspicious<br>
{<br>
/* stuff */<br>
}<br>
<br>
// vs<br>
<br>
switch (expr) {<br>
/* ToDo: finish this */<br>
} // Should warn because the emptiness of the block is suspicious<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>Here's one conceivable false positive:</div><div><br></div><div>template<typename T> void f(T t) {</div><div> switch (t) {} // produce an early diagnostic if we can't switch on 'T', this template relies on that.</div>
<div> do_complex_deeply_nested_stuff(t);</div><div>}</div><div><br></div><div>("switch (...) {}" is one of the ways we tell people to suppress the warning which is produced for "switch (...);").</div>
<div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
In the case where the user really does want an empty switch statement,<br>
they can selectively turn the diagnostic off. This really shouldn't<br>
be a common case, after all.<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Wed, May 29, 2013 at 4:10 PM, Reid Kleckner <<a href="mailto:rnk@google.com">rnk@google.com</a>> wrote:<br>
> On Wed, May 29, 2013 at 3:49 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
> wrote:<br>
>><br>
>> On Wed, May 29, 2013 at 7:35 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> wrote:<br>
>>><br>
>>> -Wunreachable-code doesn't warn on empty statement bodies for the<br>
>>> switch. So, for instance:<br>
>>><br>
>>> int main() {<br>
>>> int i = 0;<br>
>>> switch(i) {}<br>
>>> }<br>
>>><br>
>>> yields no warnings in clang with -Wunreachable-code (or -Wall), but at<br>
>>> /W3 in MSVC you get: warning C4060: switch statement contains no<br>
>>> 'case' or 'default' labels. Will it catch lots of bugs? Probably<br>
>>> not. But it also should have basically no false positives either. It<br>
>>> would be nice for people who want their code to also be warning-free<br>
>>> under MSVC.<br>
>><br>
>><br>
>> I think we need more justification for adding the warning than this.<br>
>><br>
>> It doesn't seem reasonable to expect us to implement the union of all<br>
>> warnings which every existent compiler produces. Some of those warnings are<br>
>> really dumb. For instance, /W3 also enables C4800, which I don't think we<br>
>> will ever want to implement.<br>
><br>
><br>
> I agree that warning (forcing value to bool) is really, really bad, and I<br>
> hope most serious projects turn it off.<br>
><br>
> However, I can say it's pretty annoying if you're working on cross-platform<br>
> project and have to push to another platform to build just to get some<br>
> compiler warnings for a compiler which you're expected to keep warning<br>
> clean, value judgements about its diagnostic quality aside. That cycle is<br>
> long and painful, and if you skip it people often yell at you.<br>
><br>
> While clang will never be able to paper over the differences between<br>
> platforms, having some off-by-default diagnostics like this could save users<br>
> some time and increase the likelihood that their changes Just Work on the<br>
> first push.<br>
</div></div></blockquote></div><br>