<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Mar 17, 2014 at 10:55 AM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br>
<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"><div class="">On Sat, Mar 15, 2014 at 1:04 AM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br>
<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"><div>On Fri, Mar 14, 2014 at 4:16 PM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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>
</blockquote></div><div>The downside with a builtin is that any project that wants to use this warning then needs to have a</div><div><br></div><div>#if __clang__ && __has_feature(__switch_fallthrough_macro_thinger)</div>
<div>#define FALLTHROUGH __builtin_magic_which_annotates_fallthroughs_but_might_potentially_come_in_handy_in_other_contexts</div><div>#else</div><div>#define FALLTHROUGH</div><div>#endif</div><div><br></div><div>somewhere. If you have a central header that's included ~everywhere, that's not a problem, but when your source is a collection of independent modules not sharing much code, this gets much more annoying. (You could arguably define this through a -D flag, but it's still less convenient that having the comments that everybody already uses just work.)</div>
</div></div></div></blockquote><div><br></div></div><div>I feel bad about requiring a special comment to silence the diagnostic. There are too many things, that can go wrong here: e.g. we check the placement of annotations and their correct spelling, should we do the same with comments? Should we fail, if we meet a "// FALLTHROUGH" comment somewhere outside of a switch statement?</div>
</div></div></div></blockquote><div><br></div><div>No.</div><div> </div><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">
<div> Should we allow "// FALL-THROUGH"</div></div></div></div></blockquote><div><br></div><div>Not sure, probably not? Is that in use? I've never seen this.</div><div> </div><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"><div>, "// FALL THROUGH"</div></div></div></div></blockquote><div><br></div><div>Yes (used frequently)</div><div> </div><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"><div>, "// break omitted intentionally"</div></div></div></div></blockquote><div><br></div><div>No.</div><div> </div><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"><div>, and all variations of casing</div></div></div></div></blockquote><div><br></div><div>Yes.</div><div> </div><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"><div>, spaces</div></div></div></div></blockquote><div><br></div><div>Not all variations: I'd suggest one optional space between "fall" and "through", and I'd ignore leading and trailing space.</div>
<div> </div><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"><div>, additional clarifications</div></div>
</div></div></blockquote><div><br></div><div>No. </div><div> </div><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">
<div>, usage of /* */ comments etc?</div></div></div></div></blockquote><div><br></div><div>Sure, // and /* */ comments should both work.</div><div><br></div><div>The exact details don't really matter, pick something very simple, and document this. For the code bases I looked at, the regex I proposed further up ("fall", optional space, "through", optional punctuation sign, nothing else in the comment, case-insensitive) covers the majority of fall-through comments I've seen in practice (and the others aren't feasible to detect, stuff like "// All fallthroughs in the following switch are intentional:").</div>
<div> </div><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">
<div>Using comments for this purpose would be OK for a linter tool, but it seems to be so wrong for a compiler diagnostics.<br></div></div></div></div></blockquote><div><br></div><div>It makes the warning much more convenient to use, as it makes it work out-of-the-box for code that's already annotation fallthroughs.</div>
<div> </div><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"><div></div><div>If one wants to enable the diagnostic, but is afraid of doing a global cleanup prior to this,<br>
</div></div></div></div></blockquote><div><br></div><div>I think nobody's opposed to doing a global cleanup. What is annoying having to add a FALLTHROUGH macro in N places, and replacing "// FALLTHROUGH" with "FALLTHROUGH;" seems like unnecessary busy work. (Especially given that this change will probably make previously lint-clean code be no longer lint clean.)</div>
<div> </div><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"><div> making Clang understand a certain form of the "// FALLTHROUGH" comment won't help. I don't believe it can be used consistently in the code base size of Chromium. And if it is used consistently, the clean-up would be as easy, as search-replace + insert the appropriate #include to all changed files (if there's a globally used header, it's even easier).</div>
<div><div class="h5">
<div><br></div><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">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><div><div>
<div class="gmail_quote">On 14 Mar 2014 21:08, "Richard Smith" <<a href="mailto:richard@metafoo.co.uk" target="_blank">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></div></div></blockquote></div></div></div></div></div></blockquote></div></div></div></blockquote></div></div></div><br>
</div></div>
</blockquote></div><br></div></div>