Rename compiler headers

[dnakamura]

Resolves breakage for out of tree builds. For details see eclipse-openj9/openj9#3725 & #3189

[charliegracie]

@genie-omr build all

[charliegracie]

ping @Leonardo2718 and @0xdaryl

[0xdaryl]

@Leonardo2718 and I were hemming and hawing over this because the choice of the name. We use the OMR prefix on file names to indicate that they are an extensible entity of some sort. GenerateInstructions doesn't really fit in this category.

We thought about some other solutions but nothing is really going to work in the framework that we have at the moment. Rather than make it appear like an extensible class, could you rename it to generateOMRInstructions.hpp? It's not perfect, but it makes more sense.

[rwy7]

@genie-omr build aix plinux

[dnakamura]

ping @Leonardo2718 @0xdaryl

[keithc-ca]

Perhaps this file should be named GenerateOMRInstructions.hpp - all other files in that folder start with a capital; or better yet, OMRGenerateInstructions.hpp to follow the apparent pattern of using the OMR prefix.

[Leonardo2718]

I agree with capitalizing the name: GenerateOMRInstructions.hpp. However, using OMR as a prefix was deliberately not chosen because that typically indicates the file is part of an extensible class, which GenerateInstructions.hpp is not (see @0xdaryl's comment above).

[keithc-ca]

OpenJ9 does treat the header file as extensible: it want to include the OMR header file and add extra declarations which supports my suggestion of OMRGenerateInstructions.hpp.

[charliegracie]

I feel like this file is a little strange. To me it is extensible but just not a class. If in the future a class definition was added the name would have to change again. It is so close to fitting the extensible class mould why not just go with that?

[Leonardo2718]

When @0xdaryl and I discussed this, the conclusion we came to is that the file looks "extensible" when it really shouldn't.

The fact that the GenerateInstructions.hpp files in OpenJ9 and OMR have the same name does not actually provide much value. The OpenJ9 file just includes the OMR file and defines a function, something that would also work if the files had different names. The only thing that the name-sharing allows is to avoid having to add #ifdef J9_PROJECT_SPECIFIC guards around any includes of the OpenJ9 file in OMR. However, any OMR code that uses something that is defined in the OpenJ9 file would still have to be guarded with #ifdef J9_PROJECT_SPECIFIC. AFAICT, there is only one place where this happens, and that code should be moved to OpenJ9 anyways (since it's J9_PROJECT_SPECIFIC). So, there is really little value in both files having the same name.

In fact, I think that the extensible structure of these files can make the code more confusing because figuring out what may or may not be defined requires looking across multiple files and projects, a big problem with extensible classes in general.

In addition, being able to quickly determin whether a file belongs to an extensible entity or not reduces the cognitive load of reading and reviewing code. If a file is clearly part of an extensible entity, it's an immediate sign that additional effort will be required for checking the code in the other files, namespaces, different architecture specializations, downstream projects, etc. Conversly, if a file is clearly not part of an extensible entiry, there is no need to check for the possible existence of extensions or overrides, reducing the number of "distractions" when reading the code. As a result, I strongly prefer extensible files to be clearly distinguishable from non-extensible ones.

Given the above, I think it makes sense for GenerateInstructions.hpp to not look extensible, since it doesn't need to be extensible. It's unfortunate that the existing code is structured this way and @0xdaryl and I agree that we should clean it up. For now, changing the file name to GenerateOMRInstructions.hpp is the best solution that we came up with that does not confusingly extensiblize the structure of the code while minimizing the work required to achieve the intent of this PR.

[0xdaryl]

@genie-omr build all

[0xdaryl]

Can this PR be merged without breaking anything downstream? e.g., is eclipse-openj9/openj9#6212 dependent on this PR?

[keithc-ca]

The copyright date should not be changed.

[Leonardo2718]

I believe the copyright is correct. Git is doing a poor job of showing it in the diff, but this is actually a new file. It just happens to have the same name as a file that's also renamed so git thinks the contents were moved. Looking at the diffs for each commit individually makes it easier to follow what actually happened.

[dnakamura]

@0xdaryl yes this is ready to go without breaking downstream consumers