<p dir="ltr">We may implement the solution you suggested. I can also take a look at implementing a compiler built-in-based way to silence the diagnostic. I guess, it's possible to restrict its usage to within switch statements only, unlike the hack with the labels. What do you think?</p>
<div class="gmail_quote">On 14 Mar 2014 21:08, "Richard Smith" <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Mar 14, 2014 at 10:00 AM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>The analyzer tries not to give results that are already covered by regular Clang warnings. This is sometimes problematic, since the Clang warnings may not be on by default. Nevertheless...</div>
<br><div><div><div>On Mar 11, 2014, at 19:53 , apache <<a href="mailto:ehcapa@qq.com" target="_blank">ehcapa@qq.com</a>> wrote:</div><br><blockquote type="cite"><div>----------------------------------------------------------------------</div>
<div>1.case without break</div><div>e.g.</div><div>int test(const int n) {</div><div> int ret = 0;</div><div> switch(n) {</div><div> case 1:</div><div> ret = 1;</div><div> break;</div><div> case 2:</div>
<div> ret = 2; // this case branch has no 'break' statement.(coverity gived a warning here, but Clang didn't)</div><div> default:</div><div> break;</div><div> }</div><div> return ret;</div>
<div>}</div></blockquote><div><br></div></div><div>This is supposed to be -Wimplicit-fallthrough, but I don't see it working either! Richard, do you know what's going on here?</div></div></div></blockquote><div><br>
</div><div>Yes. We originally wanted to warn here, but:</div><div><br></div><div> 1) some people are suggesting to build with -Weverything -Werror,</div><div> 2) it was thought that there was no nice way to suppress this on a case-by-case basis outside C++11 mode, and</div>
<div> 3) there were concerns that breaking non-C++11 -Weverything -Werror builds was a bad idea</div><div>so as a compromise the warning ended up disabled if you're not in C++11 mode.</div><div><br></div><div>The end result here is pretty unfortunate. I think we can do better. Here's a macro that should suppress the warning:</div>
<div><br></div><div>#define FALLTHROUGH2(LINE) goto _fallthrough_##LINE; _fallthrough_##LINE:</div><div>#define FALLTHROUGH1(LINE) FALLTHROUGH2(LINE)</div><div>#define FALLTHROUGH FALLTHROUGH1(__LINE__)</div><div><br></div>
<div>This isn't *quite* as good as "[[clang::fallthrough]];", because we would allow it outside switch statements (and in cases where it doesn't precede a case label), and because you can only put one such macro on each line, but it's a pretty good simulacrum. (Also, it doesn't suppress the warning if the use of the macro is followed by a semicolon, which seems like a bug in the warning.)</div>
<div><br></div><div>I think we should add the above macro to the documentation and remove the CPlusPlus11 check from the diagnostic. Thoughts?</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><blockquote type="cite"><div>----------------------------------------------------------------------</div><div>2.Dead code like below</div><div>#define MAX_NUM 10</div><div>
void test(const int n) {</div><div> if(n >= MAX_NUM && n < MAX_NUM) {</div><div> printf("yes\n"); // this code will never be executed!(coverity gived a warning here, but Clang didn't)</div>
<div> }</div><div>}</div></blockquote><div><br></div></div><div>This is a true deficiency in the analyzer and in -Wunreachable-code: the former isn't yet good at answering questions that are true "for all paths through the code" (dead stores being an exception), and the latter isn't smart enough to handle the combination of constraints on 'n'.</div>
<div><br></div><br><blockquote type="cite"><div><div>----------------------------------------------------------------------</div><div>3.NULL-Pointer reference like below</div><div>typedef struct {</div><div> int age;</div>
<div> int sex;</div><div>} Person;</div><div><br></div><div>Person *one_person(char flag)</div><div>{</div><div> static Person p = {0, 0};</div><div> if(flag == 1) {</div><div> return &p;</div><div> }</div>
</div><div> return 0;</div><div><div>}</div><div><br></div><div>void test()</div><div>{</div><div> Person *p = on_person(0); </div><div> p->age = 24; // NULL-Pointer reference(coverity gived a warning here, but Clang didn't)</div>
<div> p->sex = 0;</div><div>}</div></div></blockquote><br></div><div>This is a heuristic in the analyzer, assuming that in most cases you won't actually be taking the null-return case of a function you call. This is important for things like std::map::operator[], which will create a new element if the key isn't already in the map. If the value type is a pointer type, this is going to give back a null pointer and cause a spurious analyzer report.</div>
<div><br></div><div>I've had a bug on me for a while to make this work if we can <i>prove</i> that the null pointer return isn't the result of incomplete knowledge (i.e. that the analyzer didn't have to guess at the value of any of the parameters), but I haven't gotten around to it yet. It's something we have to do carefully: the analyzer has to strike a balance between false negatives and false positives, and that balance is different from Coverity's for a variety of reasons.</div>
<div><br></div><div>Thanks for the reports. If you're interested in seeing them followed up on specifically, please file bugs at <a href="http://llvm.org/bugs/" target="_blank">http://llvm.org/bugs/</a></div><span><font color="#888888"><div>
<br></div><div>Jordan</div><br></font></span></div></blockquote></div><br></div></div>
</blockquote></div>