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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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

> 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?

Sure!

  enum E { Zero, One, Two };
  
  int func(E Val) {
    switch (Val) {
    case Zero: return 12;
    case One: return 200;
    case Two: return -1;
    }
    llvm_unreachable("never get here"); // Functionally unreachable; we can't think of a reasonable way to get here without other alarms going off
  }

vs

  enum E { Zero, One, Two };
  
  extern "C" int func(unsigned Val) {
    switch (Val) {
    case Zero: return 12;
    case One: return 200;
    case Two: return -1;
    }
    assert(0 && "never get here"); // Aspirationally unreachable; we HOPE we don't get here but it's plausible that we do if someone made a logical mistake calling the API
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551