<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Oct 30, 2012, at 11:19 , David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">On Tue, Oct 30, 2012 at 11:08 AM, Abramo Bagnara<br><<a href="mailto:abramo.bagnara@bugseng.com">abramo.bagnara@bugseng.com</a>> wrote:<br><blockquote type="cite">Il 30/10/2012 18:49, David Blaikie ha scritto:<br><blockquote type="cite">On Tue, Oct 30, 2012 at 10:39 AM, Abramo Bagnara<br><<a href="mailto:abramo.bagnara@bugseng.com">abramo.bagnara@bugseng.com</a>> wrote:<br><blockquote type="cite">Il 30/10/2012 18:25, David Blaikie ha scritto:<br><blockquote type="cite">On Tue, Oct 30, 2012 at 10:19 AM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:<br><blockquote type="cite"><br>On Oct 30, 2012, at 10:17 , David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br><br><blockquote type="cite">On Tue, Oct 30, 2012 at 9:25 AM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:<br><blockquote type="cite"><br>On Oct 30, 2012, at 2:34 , Abramo Bagnara <<a href="mailto:abramo.bagnara@bugseng.com">abramo.bagnara@bugseng.com</a>> wrote:<br><br><blockquote type="cite">Il 28/10/2012 08:12, Abramo Bagnara ha scritto:<br><blockquote type="cite">$ cat p.c<br>#include <stdio.h><br><br>enum e { a, b = 4 } x = 3;<br><br>void g(int v) {<br>printf("%d\n", v);<br>}<br><br>int main(int argc, char **argv) {<br>switch (x) {<br>case a:<br> g(0);<br> break;<br>case b:<br> g(1);<br> break;<br>default:<br> g(2);<br> break;<br>}<br>}<br>$ _clang -Wunreachable-code -Wcovered-switch-default -O2 p.c<br>p.c:17:3: warning: default label in switch which covers all enumeration<br>values<br> [-Wcovered-switch-default]<br>default:<br>^<br>p.c:18:7: warning: will never be executed [-Wunreachable-code]<br> g(2);<br> ^<br>$ ./a.out<br>2<br><br>Of course -Wcovered-switch-default warning is a perfectly true positive.<br><br>My reading of the standard is that nothing prevent an enum to have a<br>value different from listed enum constants if this value is compatible<br>with enum range (and code generation seems to agree on that).<br></blockquote><br>I've attached the patch for review.<br><br>The fixed testcase shows well why to hide warnings about undefined<br>behaviour in code actually generated is a bad thing.<br></blockquote><br>If we do this, we're going to want this under a CFG option at the very least. The static analyzer should continue assuming that an enum input to a switch is always a valid enum constant, in order to keep our false positive rate down.<br></blockquote><br>Yeah, I doubt this'll be any better in the compiler proper, really.<br>The heuristic exists to, as you rightly point out, reduce false<br>positives & that rationale exists in the compiler as well.<br><br>While, yes, it means we lose some true positives as well, that<br>probably isn't worth the increase in false positives.<br></blockquote><br>I can see Abramo's point, however, that in a more defensive coding style the current -Wunreachable warning can easily be considered a false positive. We don't optimize out the default case in an enum-covered switch.<br></blockquote><br>Flagging this as unreachable code is a bug & should be fixed - but<br>probably in the way I described. Treating it purely as reachable code<br>& emitting our 'runtime' diagnostics for code in such situations will<br>(I believe - though I haven't run numbers) increase false positive<br>rates substantially.<br><br>A trivial example that GCC often warns about & Clang deliberately does not:<br><br>int func(enum X v) {<br> switch (v) {<br> case A: return 1;<br> case B: return 2;<br> ... // fully covered<br> case Z: return 26;<br> }<br> // GCC warns that the function may exit without a return value here,<br>Clang does not<br> // the LLVM/Clang codebase has a lot of llvm_unreachables after<br>fully covered/exiting<br> // switches like this to silence GCC's warning. Each of those is,<br>essentially, a GCC<br> // false positive (in the sense that the code is not buggy).<br>}<br></blockquote><br>:-o<br><br>Unless I'm missing something, the code is definitely buggy and leads to<br>undefined behaviour in C++ entering with v = Z + 1. Note that entering<br>with v = Z + 1, is not per se an undefined behavior: only the missing<br>return causes that.<br><br>Can we at least agree on that?<br></blockquote><br>Yes & no. Yes a program exhibits UB if the function is called with v =<br>Z + 1, no the code is not (necessarily) buggy if the function is never<br>called with such invalid values.<br><br>If code is written in such a way as to not violate that invariant,<br>then the warning is a false positive (it has not found a bug in the<br>code). If people often write code with this invariant then the false<br>positive rate is "high" and the true positive rate is "not so high",<br>so we try to avoid warning & producing more noise than good advice.<br>(it's obviously not a 1:1 ratio, and it's certainly a judgement call)<br><br><blockquote type="cite">If we'd agree on that we will easily discover that my proposed change<br>does not lead to any false positive diagnostics, that GCC is right and<br></blockquote><br>Your definition of "false positive" differs from mine/ours. Hopefully<br>my description above helps describe the motivation here.<br></blockquote><br>Yes, my definition of false positive is different, see:<br><br><a href="http://en.wikipedia.org/wiki/Type_I_and_type_II_errors#False_positive_error">http://en.wikipedia.org/wiki/Type_I_and_type_II_errors#False_positive_error</a><br><br>"is the default unreachable"? ("is there a wolf near the herd?")<br><br>If the message says "warning: will never be executed<br>[-Wunreachable-code]" ("Wolf, wolf!") then we have a false positive.<br><br>The idea that a warning is a false positive if it has not found a bug in<br>the code is rather weird... do you know any warning that is able to<br>always find a bug without knowing the programmer intentions?<br></blockquote><br>Generally, no. They make local assumptions so to make a best effort at<br>finding bugs. Part of that is also to avoid finding non-bugs because<br>doing so creates a burden on developers that may eventually eclipse<br>the benefit they gain from the warning.<br><br>We're not trying to write warnings to teach people about the semantics<br>of their code - they can read books about that. They're meant to be<br>actionable for a good reason, not just to indicate that the developer<br>read & understood the diagnostic message & then went on their merry<br>way.<br></blockquote><div><br></div><div>We try to guess people's intentions all the time. -Wmissing-semi will insert the semicolon and continue compiling as if it had been there. -Wparentheses is a controversial style warning, but it catches real bugs. We don't warn about tautological comparisons in templates because many times the template itself isn't tautological, only the instantiation.</div><div><br></div><div>When talking about the analyzer I've sometimes called behavior like this "pedantically true"—cases where something <i>could</i> happen and we have no way to check it within this translation unit, but it most likely <i>won't</i> and programmers shouldn't have to spend time thinking about it. That applies in both directions: we shouldn't give a -Wreturn-type warning if there's a fully-covered enum switch, but we shouldn't call it out as being unreachable either. So I agree with your latest e-mail in the thread.</div><div><br></div><div>(The llvm_unreachable does allow for optimization that we wouldn't get otherwise, so we probably should stop calling it a sop to GCC.)</div><div><br></div><br><blockquote type="cite"><br><blockquote type="cite">What about enum designed to be used to represent a mask?<br></blockquote><br>They happen, certainly. I'm not at all claiming that people never<br>deliberately put values outside the enum constants into an enum value.<br></blockquote><div><br></div><div>This is legal in C++11 and probably in C11 as well, but for what it's worth, doing a switch on a value whose type is an enum of bit-flags is probably almost always the wrong thing to do.</div></div><div><br></div></body></html>