<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>