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

Tue Oct 11 10:46:49 PDT 2022

inclyc added a comment.

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

> In D135551#3849944 <https://reviews.llvm.org/D135551#3849944>, @inclyc wrote:
>
>>> - Prefer llvm_report_error() in any circumstance under which a code path is functionally possible to reach, but only in erroneous executions that signify a mistake on the part of the LLVM developer elsewhere in the program.
>>> - Prefer llvm_unreachable() in any circumstance under which a code path is believed to be functionally impossible to reach (even if technically possible to reach). The API is now self-documenting to mean "this code really should be totally unreachable".
>>
>> I think `llvm_unreachable` already has the functionality reporting bugs for developer in our implementation, with +Assertions by default
>
> Yes, in terms of its runtime behavior. So long as we're not making debugging harder for some large group of people, the runtime behavior is not really what I'm concerned by though. I'm focusing more on code reviewers and project newcomers and whether our code is self-documenting or not. Having a policy to use an API that says code is not reachable in situations where that code is very much reachable is the crux of my problem -- the API is sometimes a lie (and a lie with optimization impacts, at that) and we force everyone to pay the cognitive costs associated with that when reading code.
>
> If we end up with two APIs that have the same runtime behavior, I'm okay with that.

Could you elaborate on "aspirationally" vs "functionally" unreachable code here?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551