[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

[YingChi Long via Phabricator via cfe-commits]

inclyc added a comment.

In D135551#3846592 <https://reviews.llvm.org/D135551#3846592>, @aaron.ballman wrote:

> I don't know if that discussion reached a conclusion to move forward with this change -- my reading of the conversation was that efforts would be better spend on fuzzing instead of changing policy about using unreachable vs assert(0).
>
> In general, I'm a bit hesitant to make this change. On the one hand, it's logically no worse than using `assert(0)` in a release build (if you hit this code path, bad things are going to happen). But `__builtin_unreachable` can have time travel optimization effects that `assert` doesn't have, and so the kind of bad things which can happen are different between the two (and use of unreachable on reachable code paths might make for harder debugging in RelWithDebInfo builds). Historically, we've usually used `llvm_unreachable` for situations where we're saying "this code cannot be reached; if it can, something else has gone seriously wrong." For example, in code like: `int foo(SomeEnum E) { switch (E) { case One: return 1; default: return 2; } llvm_unreachable("getting here would be mysterious"); }` and we've used `assert(0)` for situations where we're saying "this code is possible to reach only when there were mistakes elsewhere which broke invariants we were relying on." The two situations are similar, but still different enough that I don't think we should wholesale change from one form to another.



> but still different enough that I don't think we should wholesale change from one form to another.

In general we can control the behavior here via `-DLLVM_UNREACHABLE_OPTIMIZE` to choose making assumptions or traps (looks better than assertions to me).

https://github.com/llvm/llvm-project/blob/50312ea133999cb2aad1ab9ef0ec39429a9427c5/llvm/include/llvm/Support/ErrorHandling.h#L125

(This change was landed 7 months ago https://reviews.llvm.org/D121750)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135551/new/

https://reviews.llvm.org/D135551