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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

> flang is not different. I think a developer in any part of the project should have the option of using a version of llvm_unreachable (called DIE or something else) that is defined to produce an internal error even in Release builds.

This is not without bound: a developer has the option to use it *withing the guidelines*. Here is just isn't in line with the LLVM coding standard and the LLVM usual use of asserts/unreachable.

> I object to this particular change because whoever wrote the code chose to use DIE, which has well-defined behavior in Release builds and you haven't given a good reason (IMO) to override that decision and make it undefined.
>  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.

The reason is not the name, otherwise we would rename it as report_fatal_error which should provide the same functionality as DIE. The reason it is the policy given above (link to coding standard on assert) + the fact that this was a condition for flang to merge in LLVM as I pointed out above: the ticket was part of the dashboard of things that will be fixed in-tree after landing.

You can't use the fact that DIE was used before flang was in merged in LLVM to prevent updating it, while this was identified before merge as something that would need to change.

I'm clearly +1 with David here:

> It represents a distinct philosophy about error handling that isn't one shared by the LLVM project (as described in the LLVM coding standards and programmers guide). I appreciate different folks have different takes on this - but the LLVM project is pretty clear on this in its documentation, and in communications with the flang community prior to accepting the project.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79507