<div dir="ltr">On Mon, Jun 3, 2013 at 9:30 AM, Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div>Should this have a -W flag?</div></div></blockquote><div><br></div><div>Every warning should have some -W flag. There's a test that checks for that. (test/Misc/warning-flags.c)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div> Should it be on or off by default?<br></div></div></blockquote><div><br></div><div>The guideline so far was "if it's not useful enough to be on by default, it probably shouldn't be in clang".</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div></div><div>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.<br>
</div><div><br></div>The kinds of time wasters I used to hit are probably covered by -Wconversion and -Wsign-compare, but I never turned them on.<div><br></div><div>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.</div>
</div></blockquote><div><br></div><div>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).</div>
<div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">
<div><br></div><div>Then this switch warning and others like it could go under -Wmsvc-3, without leading to a proliferation of -Wreally-obscure-warning flags.</div><div><br></div><div>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.</div>
</div><div class=""><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jun 3, 2013 at 11:23 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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>
<br>
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>
<br>
<br>
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><font color="#888888"><br>
~Aaron<br>
</font></span><div><div><br>
On Wed, May 29, 2013 at 4:10 PM, Reid Kleckner <<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>> wrote:<br>
> On Wed, May 29, 2013 at 3:49 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">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" target="_blank">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></div>
</div></div><br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div>