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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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

> In D135551#3846592 <https://reviews.llvm.org/D135551#3846592>, @aaron.ballman wrote:
>
>> I don't know if that discussion reached a conclusion to move forward with this change -- my reading of the conversation was that efforts would be better spend on fuzzing instead of changing policy about using unreachable vs assert(0).
>>
>> In general, I'm a bit hesitant to make this change. On the one hand, it's logically no worse than using `assert(0)` in a release build (if you hit this code path, bad things are going to happen). But `__builtin_unreachable` can have time travel optimization effects that `assert` doesn't have, and so the kind of bad things which can happen are different between the two (and use of unreachable on reachable code paths might make for harder debugging in RelWithDebInfo builds). Historically, we've usually used `llvm_unreachable` for situations where we're saying "this code cannot be reached; if it can, something else has gone seriously wrong." For example, in code like: `int foo(SomeEnum E) { switch (E) { case One: return 1; default: return 2; } llvm_unreachable("getting here would be mysterious"); }` and we've used `assert(0)` for situations where we're saying "this code is possible to reach only when there were mistakes elsewhere which broke invariants we were relying on." The two situations are similar, but still different enough that I don't think we should wholesale change from one form to another.
>
>
>
>> but still different enough that I don't think we should wholesale change from one form to another.
>
> In general we can control the behavior here via `-DLLVM_UNREACHABLE_OPTIMIZE` to choose making assumptions or traps (looks better than assertions to me).
>
> https://github.com/llvm/llvm-project/blob/50312ea133999cb2aad1ab9ef0ec39429a9427c5/llvm/include/llvm/Support/ErrorHandling.h#L125
>
> (This change was landed 7 months ago https://reviews.llvm.org/D121750)

That doesn't change the underlying concern that `assert(0)` and `unreachable` are used for different purposes and trying to unify those use cases might lose some expressivity in the code base.

I've left some comments in the review about examples of my concerns (it's not an exhaustive review).



================
Comment at: clang/tools/libclang/CIndex.cpp:5191
 
-  assert(false && "Invalid CXPrintingPolicyProperty");
+  llvm_unreachable("Invalid CXPrintingPolicyProperty");
   return 0;
----------------
This one is a bit questionable -- this is part of the C interface we expose, which is ABI stable, so the assert was alerting users to potential mismatches between versions of the library.


================
Comment at: clang/tools/libclang/CXCursor.cpp:1490
       CXGetTemplateArgumentStatus_Success) {
-    assert(0 && "Unable to retrieve TemplateArgument");
+    llvm_unreachable("Unable to retrieve TemplateArgument");
     return 0;
----------------
Each of these is actually reachable -- the asserts exist specifically to tell users of the C interface about problems with their assumptions. In each of these cases, the assert is avoiding the need for a local variable to assert on.


================
Comment at: clang/utils/TableGen/ClangSyntaxEmitter.cpp:120
     } else {
-      assert(false && "Unhandled Syntax kind");
+      llvm_unreachable("Unhandled Syntax kind");
     }
----------------
This should not be using unreachable -- the code is very much reachable. This should have changed from `assert` to `PrintFatalError`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551