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

Tue Oct 11 09:24:50 PDT 2022

aaron.ballman added a comment.

In D135551#3849816 <https://reviews.llvm.org/D135551#3849816>, @dblaikie wrote:

>>> Generally LLVM's pretty hard to fathom in a non-asserts build anyway, right? (that's the first thing any of us do is reproduce with an assertions build that may fail miles away from where a crash occurred because an invariant was violated much earlier) - that `cast` won't crash/will continue on happily in a non-asserts build seems like a much larger hole to debuggability of a non-asserts build than any unreachable?
>>
>> This might be true -- personally, I tend to only use debug builds with MSVC because RelWithDebInfo isn't sufficient for my daily needs. However, I've definitely heard of folks who use RelWithDebInfo for their daily work (RelWithDebInfo + Asserts specifically, IIRC) because of the improved build times and runtime performance; we should be sure we're not disrupting that workflow too much.
>
> Changing `assert(0)` to `llvm_unreachable` does not change the behavior of any +Asserts build. The behavior of unreachable is the same as assert(0) in a +Asserts build.
>
> Is this observation enough to undeadlock this conversation?

No, it doesn't address the key point I have which is that I want different APIs to express intent instead of using `llvm_unreachable` in all cases. To me, it is a mistake to label functionally reachable code as being unconditionally unreachable. It requires everyone reading that code to understand our nuances to know that the API signals aspirationally unreachable code rather than functionally unreachable code. These are different scenarios and I don't want to lose that distinction in the places where it matters.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551