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

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:598
       } else {
-        assert(false && "Unhandled type in array initializer initlist");
+        llvm_unreachable("Unhandled type in array initializer initlist");
       }
----------------
aaron.ballman wrote:
> dblaikie wrote:
> > aaron.ballman wrote:
> > > The rest of the ones here are somewhat interesting in that the interpreter is an experiment under active development and is known to be incomplete. In all of these cases, I think the switch to unreachable is flat-out wrong -- these asserts serve explicitly to find unimplemented cases when we hit them.
> > & I don't see why unreachable is any different a statement than assert(false) in these cases... - it's the same statement of intent. "if this is reached you've found a bug" (in this case, a missing feature)
> > 
> > But I'd be sort of OK changing all these to report_fatal_error. But, again, I think the assert(false) -> unreachable is a valid transformation and doesn't make anything worse than it already is, but improves the code by being more consistent and removing this confusion that there might be something different about assert(false) when, I believe, there isn't.
> > & I don't see why unreachable is any different a statement than assert(false) in these cases... - it's the same statement of intent. "if this is reached you've found a bug" (in this case, a missing feature)
> 
> You are asserting it's the same statement of intent and I keep pointing out that people use the different constructs in practice because they're different statements of intent. I don't know how to resolve this difference of opinion, but I can say as someone doing code review in this area recently that your interpretation is wrong according to what we were after with this code.
> 
> I'd be fine changing it to `report_fatal_error` instead of `assert(false)`; I'd be strongly opposed to switching to `llvm_unreachable`.
I use llvm_unreachable as a nicer to use assert in if/else chains like this. I also see no difference in the intent between assert and unreachable; assert(0 && "message") is just uglier.

report_fatal_error is for something a user could plausibly run into but also isn't worth wiring into a proper error diagnostic (which happens a lot in codegen)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551