[PATCH] D79507: [flang] Change DIE("unreachable") cases to use llvm_unreachable

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

I feel like there are a number of conflated issues here.

We should use `llvm_unreachable` in cases consistent with how LLVM and Clang use `llvm_unreachable`. I describe these as are places where, if the logic of the code is correct, no user input should trigger the condition even though that might not be locally obvious. In release mode, where we do care about the performance of the Compiler's -fsyntax-only mode, these are compiled out allowing for more-aggressive optimization.

The string, however, in `llvm_unreachable` should indicate why the condition should not have been reached, so that if it is reached during debugging, it's easier to figure out where the bug is. We should not have `llvm_unreachable("unreachable")`. `llvm_unreachable("can't happen");` is not good either.

For the cases like this:

  bool Pre(const parser::MainProgram &) { DIE("unreachable"); }

are these unreachable because they correspond to things that just haven't been implemented yet? If so, the message should say something about being unimplemented (and I don't see much value in making these `llvm_unreachable` because we might want them to trigger explicitly even if we try compiling things in Release mode).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79507