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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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

> In D79507#2067070 <https://reviews.llvm.org/D79507#2067070>, @DavidTruby wrote:
>
> > I believe consistency is key, which is why I was pushing this change in the first place. Many of us work on multiple LLVM sub-projects and having to remember different coding guidelines for different files in the same repository is fairly irritating, and I think there's a barrier to entry for getting people from other subprojects to help us fix things if we're going to be rejecting their patches to flang because they don't follow the written guidelines the whole project should follow and the style that the subproject they usually work on follows.
>
>
> flang has no guidelines that say don't use llvm_unreachable and no changes have been rejected because of its use.


Except this one. Flang's implicit guidelines seem to have some idea about when DIE is appropriate and distinct from llvm_unreachable.

>> I haven't seen a reason articulated as to why this change shouldn't be made that doesn't apply to the project as a whole; I'd like to know what makes flang exceptional here that we should do things differently.
> 
> 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.

That is different from the rest of LLVM - it's a way you would like the LLVM project to work, but it's not the way it works at the moment & flang was accepted into the LLVM project with an explicit understanding that this part of the LLVM style would be adhered to.

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

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