<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head>
<body>
<div style="FONT-SIZE: 11pt; FONT-FAMILY: Calibri,sans-serif">I've had this discussion with Dave before, mostly because of code reviews where people add asserts that don't fit the definition of assert that we both seem to have.<br>
<br>Assert usually means one of two things to people:<br><br>1. Logically, this condition should never happen and if it does, fixing it will require a code change. E.g. A switch attempting to handle all possible values of an enum.<br>
2. In most reasonable cases, this condition shouldn't happen, but if it does, tell me, though it might not result in a code change.. E.g. Asserting that clang++ is a symlink to clang in the same directory (I've seen people do the equivalent of this).<br>
<br>I personally think asserts are only 1. And if that view is consistent with all of the asserts in llvm/clang, then the following would seem reasonable:<br><br>If asserts are enabled, then they have to be guaranteed to print their message and all code before then should be well defined. Code after them, however is suspect, because the program is now, by definition, in an illogical state.<br>
<br>If asserts are disabled, then the optimizer may use them to assume those code paths don't exist. This would imply that asserting is treated like undefined behavior.<br><br>Practically, a codebase the size of llvm probably has lots of asserts that don't fall into the 'logical' error categorization above and therefore, blanket changing all of them to fit that definition is risky. I think having that definition of assert is useful and perhaps having another construct to represent the gray area of 2 above.</div>
<hr>
<span style="FONT-WEIGHT: bold; FONT-SIZE: 10pt; FONT-FAMILY: Tahoma,sans-serif">From: </span><span style="FONT-SIZE: 10pt; FONT-FAMILY: Tahoma,sans-serif">Chandler Carruth</span><br><span style="FONT-WEIGHT: bold; FONT-SIZE: 10pt; FONT-FAMILY: Tahoma,sans-serif">Sent: </span><span style="FONT-SIZE: 10pt; FONT-FAMILY: Tahoma,sans-serif">Wednesday, September 21, 2011 5:24 PM</span><br>
<span style="FONT-WEIGHT: bold; FONT-SIZE: 10pt; FONT-FAMILY: Tahoma,sans-serif">To: </span><span style="FONT-SIZE: 10pt; FONT-FAMILY: Tahoma,sans-serif">David Blaikie</span><br><span style="FONT-WEIGHT: bold; FONT-SIZE: 10pt; FONT-FAMILY: Tahoma,sans-serif">Cc: </span><span style="FONT-SIZE: 10pt; FONT-FAMILY: Tahoma,sans-serif"><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a></span><br>
<span style="FONT-WEIGHT: bold; FONT-SIZE: 10pt; FONT-FAMILY: Tahoma,sans-serif">Subject: </span><span style="FONT-SIZE: 10pt; FONT-FAMILY: Tahoma,sans-serif">Re: [cfe-commits] r140232 - /cfe/trunk/lib/Sema/SemaExpr.cpp</span><br>
<br></body></html><div class="gmail_quote">On Wed, Sep 21, 2011 at 5:07 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div>I'm not trying to be obtuse, but that isn't entirely clear to me that in such cases we want to gracefully fall back - in clang code itself I've seen cases much like that (...) where a set of values are handled but the default contains an assert. This assert is generally a "if we have a FOO_Crazy here, I don't know what's going on" - eg: an invalid argument, or the like. <br>
<br>One example of this I came across is, for example, in tools/libclang/CIndexUSRs.cpp, line 320-ish. USRGenerator::VisitObjCContainerDecl - it switches over the Kind, but expects only to find a subset of Kinds because it has a particular subclass of Decl. It asserts (really should be llvm_unreachable) on any other kind because it would be totally bogus.</div>
</blockquote><div><br></div><div>This seems like a case very different the one I'm citing. It truly is an invariant, and not one that is reasonably recoverable.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div> This doesn't hide any bugs, and it doesn't cause any problems.</div>
</div></blockquote><div><br></div></div><div>What I said about hidden bugs is that these code paths - not only the ones from the assert onwards, but the ones prior to the assert that trigger it, are untested. We haven't tested any code path that leads to this function being passed FOO_Crazy - so if we reach that state it's not safe to say we can just carry on as though it's just FOO_InvalidN.</div>
</blockquote><div><br></div><div>There are a lot of untested code paths, but assert bearing ones aren't always untested. We have some tests that only pass when asserts are enabled for exactly this reason. Just because the code path isn't tested doesn't mean we should crash. We might have a very reasonable expectation of handling the result.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div>While, yes, a developer could make a clear decision about this "Here's something I think might be true, and for my own sanity I'd like to know about it if it's not true, but if it isn't true I might be able to handle it" I think that's a really questionable thing to try to enshrine in code.</div>
</blockquote><div><br></div><div>Well, from what I have seen in response to some patches of mine, the authors of some significant chunks of LLVM have made exactly this decision. We could debate the merits of it, and we can debate whether it is the optimally clean design, but that debate should first be informed by looking at the *actual* code, and learning the context in which the decision was made. Then, bring it up on code review *for that code*.</div>
<div><br></div><div>I don't think trying to do a blanket change from one practice to another without contextually examining the practices is really viable. Its especially not viable if this comes down (fundamentally) to developer preferences.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div>I realize this isn't what everyone means when they say assert, evidently, & I'm not doing a terribly good describing why I think that point of view is incorrect/unhelpful.</div>
</blockquote></div><br><div>I think you should try to take this discussion up again the next time you review a patch which actually adds such a construct. There you and the author will both ave the context to debate the issue and dig into why or why not its the best solution.</div>
<div><br></div><div>On that note, we should perhaps move on from this discussion. I think we're just restating our points. I'm hopeful that perhaps revisiting the discussion in such a context with a concrete case in mind will be more productive.</div>