[cfe-commits] r140232 - /cfe/trunk/lib/Sema/SemaExpr.cpp

[Chandler Carruth]

On Wed, Sep 21, 2011 at 5:07 PM, David Blaikie <dblaikie at gmail.com> wrote:

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

This seems like a case very different the one I'm citing. It truly is an
invariant, and not one that is reasonably recoverable.


>
>
>> This doesn't hide any bugs, and it doesn't cause any problems.
>>
>
> 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.
>

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.

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

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

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.

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

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.

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110921/0bc2a566/attachment.html>