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

[David Truby via Phabricator via llvm-commits]

DavidTruby added a comment.

In D79507#2074264 <https://reviews.llvm.org/D79507#2074264>, @tskeith wrote:

>   The reason for the change seems to be that someone new to flang might encounter DIE and not know at first what it does (!?); the same would seem to be true of thousands of other names they would encounter.


This is neither what I said nor what I meant. My argument about consistency is that someone working in the LLVM project and who has read and is familiar with the LLVM Coding Guidelines should feel free to apply those guidelines when crossing between subprojects. That would mean that the use of `assert` and `llvm_unreachable` as is strongly stated in those guidelines, regardless of whether this subproject thinks that debug-only assertions are acceptable or not. It's entirely possible that someone working in a file flang may have never seen the DIE macro and it may not be present in the file in which they're working. Should they still have to know it exists in this case?

It seems clear to me that if this change I'm proposing (which, might I reiterate, only actually introduces `llvm_unreachable` in cases that are either demonstrably not reachable or are stated to not be reachable in comments) is rejected on these grounds then any patch containing `llvm_unreachable` is likely to be rejected for the same reason; especially if it's being used in a case that is believed to be, but isn't provably, unreachable (which would still be in line with the LLVM Coding Guidelines suggested usage).
The argument that "this hasn't happened yet so it isn't a problem" is not one I think has any relevance if we actually intend to increase the number of people working on this subproject.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79507