Rename extensible header files in the the compiler

[dnakamura]

A number of header files in the compiler are designed to be extened by
the consuming runtime. For these headers we prefix the names with OMR
and provide a new default implementation of the header which simply
includes the OMR prefixed header. (this is in line with existing
extensible headers)

Signed-off-by: Devin Nakamura [email protected]

[charliegracie]

The Lint build breaks with this change

[0xdaryl]

Your commit titles have a double "the". Please fix.

[Leonardo2718]

Changes LGTM.

Is there a plan to make similar changes to the corresponding files in downstream projects (e.g. OpenJ9)?

Also, was there any public discussion for why this change is wanted/needed?

[dnakamura]

@Leonardo2718 Yes plan is to make changes to downstream projects. Changes are being made so we can do out of tree builds properly (where OMR isnt a sub-directory of consuming project)

[Leonardo2718]

Changes are being made so we can do out of tree builds properly (where OMR isnt a sub-directory of consuming project)

@dnakamura Could you open an umbrella issue for this, so that others can more easily track what's going on?

[Leonardo2718]

@genie-omr build all

[charliegracie]

ping @dnakamura and @Leonardo2718

[0xdaryl]

The reason the OMR compiler needs to smurph its filenames is in support of the extensible class mechanism. Arguably, it doesn't look particularly clean to use the project name "OMR" in the filename of the very project it's used in, but it was needed to work around some of the limitations of the C preprocessor. Hence, I would like as much as possible to do this only where necessary to support extensible classes.

Many of the files involved in this PR aren't extensible classes per se, but do fall in some kind of grey area where they are needed to support the extensible mechanism (e.g., filling in the internals of an extensible table or enum). Therefore, I think I can get behind most of these proposed changes.

However, for runtime/Runtime.hpp, this is neither an extensible class nor anything that depends on the extensibility mechanism. Rather, it's an eclectic collection of bits and pieces needed to support compiled code at runtime. I don't see the rationale for smurphing it with OMR, as it gives the appearance that it's part of the extensibility mechanism--which it isn't. If a downstream project is consuming it with the intent on extending it then I'd argue that's not an appropriate (or intended) use of it, and I suggest solving that in the downstream project instead.

[dnakamura]

@0xdaryl I saw downstream consumer extending it in the same manner as the normal extensible headers. I assumed that they were relying on the fact that the omr headers will include "Runtime.hpp" and get their copy (which then includes the omr copy), but I will do a bit more digging to see if that is the case

[charliegracie]

@dnakamura ping?

[dnakamura]

For some reason I thought this pr was only dealing with Runtime.hpp which is why i closed it, however there are other useful changes here

[Leonardo2718]

In extensible classes, we normally do not use "compiler/" at the start of includes (there is one exception but it doesn't apply in this case):

Suggested change

	#include "compiler/il/OMRILProps.hpp"
	#include "il/OMRILProps.hpp"

Is there a reason for needing them here?

[dnakamura]

just an oversight on my part

[Leonardo2718]

compiler/aarch64/codegen/TreeEvaluatorTable.hpp and compiler/arm/codegen/TreeEvaluatorTable.hpp still have compiler/ in the includes.

[dnakamura]

@0xdaryl poke

[Leonardo2718]

LGTM 👍

[Leonardo2718]

@genie-omr build all

[DanHeidinga]

Whats the next step to get this merged? I see the builds have been run and succeeded.

[dnakamura]

@DanHeidinga I believe its just waiting for someone to click the merge button. I believe I have addressed everyone's issues

[Leonardo2718]

All comments have been addressed. Everyone who reviewed has approved. All checks have passed. Merging.