-
Notifications
You must be signed in to change notification settings - Fork 398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework extensible enums #3189
Rework extensible enums #3189
Conversation
Rename the base enum files to be OMR<name>.enum This is in line with the naming for headers and extensible classes Signed-off-by: Devin Nakamura <[email protected]>
d2b488a
to
256574e
Compare
Signed-off-by: Devin Nakamura <[email protected]>
256574e
to
c7c35ed
Compare
@genie-omr build all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good!
Other extensible enums in the compiler use .hpp
as extension, not .enum
. Can you change the names of the new files so they match that convention?
@@ -1,5 +1,5 @@ | |||
/******************************************************************************* | |||
* Copyright (c) 2000, 2016 IBM Corp. and others | |||
* Copyright (c) 2018, 2018 IBM Corp. and others |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this correct?
@@ -1,5 +1,5 @@ | |||
/******************************************************************************* | |||
* Copyright (c) 2000, 2018 IBM Corp. and others | |||
* Copyright (c) 2018, 2018 IBM Corp. and others |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again I am wondering why the initial copyright date was changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git didnt really track the changes properly. DataFlowAnalysis,enum was renamed to OMRDataFlowAnalysis.enum, and a new file was created with the old name. I was hoping that by keeping the rename in one commit, and the creation of the new file in a separate commit, git would properly track the file move, but that seems to have failed
@Leonardo2718 Can you point to an instance where the enum is in a .hpp file? I'm hesitant to make that change since its not really valid as a standalone header. From what I can tell they are designed to be used like so: SomeEnum.hpp:
|
That usage is correct. Other examples of extensible enums that
They're fairly easy to find by grepping for files with
|
Are there other *.enum files already? To be clear @dnakamura did not create the |
The files modified in this PR are the only ones I know of that use That said, I do see why using |
ping @Leonardo2718 |
As I mentioned in #3213, I would like to see an umbrella issue open to track why these changes are needed. The extensible enum mechanism has historically been the source of much discussion so I want to make sure we're tracking the reasons for changing their structure. |
My preference going forward is to not use I support the renaming to an |
@dnakamura ping? |
@charliegracie I believe opening an umbrella issue was the only outstanding request on this PR. Feel free to correct me if I am mistaken |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Use renamed enums from omr see eclipse-omr/omr#3189 Signed-off-by: Devin Nakamura <[email protected]>
Use renamed enums from omr see eclipse-omr/omr#3189 Signed-off-by: Devin Nakamura <[email protected]>
Currently, the way extensible enum files are named, requires having
my_project/compiler/Foo.enum:
However, this approach breaks if the path to omr sources ever changes. Instead this PR restructures
extensible enums to work the same way as extensible classes. ie
my_project/compiler/Foo.enum: