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

Tue Oct 11 11:34:43 PDT 2022

dblaikie added a comment.

In D135551#3850266 <https://reviews.llvm.org/D135551#3850266>, @inclyc wrote:

> This makes sense! However I think `assert(0)` should not be used in this case, we could expose another `llvm_unreachable`-like api and probably `llvm_report_error` shall be fine. Are there some changed assertions actually "Aspirationally unreachable" in this patch?

No, I really don't think  we should go down that path.

I believe these are not actually distinct cases - in either case, the program has UB if they violated the invariants/preconditions - whether or not they called through the C API.

unreachable is no more a guarantee/proven thing than an assertion - both are written by humans and a claim "if this is reached-or-false, there is a bug in some code, somewhere". The statement is not stronger in the unreachable case and the style guide supports that perspective and the way we triage/treat bugs is pretty consistent with that - we get bugs all the time when an unreachable is reached and that doesn't seem to surprise most/anyone - we treat it the same as a bug when an assertion fires.

The discourse discussion, I think, supports this ^ perspective.

As there's still disagreement, should this escalate to the RFC process to change the style guide, Aaron?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551