Skip to content
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

Merged
merged 2 commits into from Dec 12, 2018
Merged

Conversation

dnakamura
Copy link
Contributor

Currently, the way extensible enum files are named, requires having

my_project/compiler/Foo.enum:

#include "path_to_omr/compiler/Foo.enum"

SOME_ENUM_VAL
OTHER_ENUM_VAL
...

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:

#include "compiler/OMRFoo.enum"
...

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 <devinn@ca.ibm.com>
Signed-off-by: Devin Nakamura <devinn@ca.ibm.com>
@Leonardo2718
Copy link
Contributor

@genie-omr build all

Copy link
Contributor

@Leonardo2718 Leonardo2718 left a 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?

@0xdaryl 0xdaryl changed the title Compiler: Rework extensible enums Rework extensible enums Nov 14, 2018
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2000, 2016 IBM Corp. and others
* Copyright (c) 2018, 2018 IBM Corp. and others
Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Contributor Author

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

@dnakamura
Copy link
Contributor Author

@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:

enum SomeEnum{
#include "SomeEnum.enum"
};

@Leonardo2718
Copy link
Contributor

@dnakamura

That usage is correct. Other examples of extensible enums that .hpp are:

  • compiler/il/OMRILOpCodesEnum.hpp
  • compiler/il/DataTypesEnum.hpp
  • compiler/codegen/OMRRecognizedMethodsEnum.hpp
  • compiler/codegen/OMRInstructionFlagEnum.hpp
  • compiler/optimizer/OMRSimplifierTableEnum.hpp
  • compiler/{p,z,arm,aarch64,x}/codegen/OMRInstructionKindEnum.hpp'

They're fairly easy to find by grepping for files with Enum in their name.

.hpp may not be the best extension but I think it's the most used. If we decide to change it to something else later on, it should be easy enough to change it on all the files.

@charliegracie
Copy link
Contributor

Are there other *.enum files already? To be clear @dnakamura did not create the .enum files but rather just changed their names to make it easier to work with.

@Leonardo2718
Copy link
Contributor

Are there other *.enum files already? To be clear @dnakamura did not create the .enum files but rather just changed their names to make it easier to work with.

The files modified in this PR are the only ones I know of that use .enum. My thinking was that since the files are being renamed to follow the pattern of other extensible enums (and classes), also changing the file extensions so they match makes sense.

That said, I do see why using .enum makes sense. If others don't see value in changing the extension, I'll accept the changes as they currently are.

@charliegracie
Copy link
Contributor

ping @Leonardo2718

@Leonardo2718 Leonardo2718 self-assigned this Nov 26, 2018
@Leonardo2718
Copy link
Contributor

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.

@0xdaryl
Copy link
Contributor

0xdaryl commented Nov 27, 2018

My preference going forward is to not use .hpp file extensions for entities that are used to compose enums and static tables. The reason is they are not complete header files and can only be included in very carefully guarded circumstances (e.g., within the braces of an enum). A different file extension makes that clear.

I support the renaming to an .enum extension for the enum files and think it should proliferate throughout OMR. I also think we need a different extension for the static table files (e.g., .table or .init).

@charliegracie
Copy link
Contributor

@dnakamura ping?

@dnakamura
Copy link
Contributor Author

#3328

@dnakamura
Copy link
Contributor Author

@charliegracie I believe opening an umbrella issue was the only outstanding request on this PR. Feel free to correct me if I am mistaken

Copy link
Contributor

@Leonardo2718 Leonardo2718 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@Leonardo2718 Leonardo2718 merged commit 325c987 into eclipse:master Dec 12, 2018
@dnakamura dnakamura deleted the compiler_enums branch January 14, 2019 19:30
dnakamura added a commit to dnakamura/openj9 that referenced this pull request Jan 24, 2019
Use renamed enums from omr see eclipse/omr#3189

Signed-off-by: Devin Nakamura <devinn@ca.ibm.com>
Yuehan-Lin pushed a commit to Yuehan-Lin/openj9 that referenced this pull request Feb 5, 2019
Use renamed enums from omr see eclipse/omr#3189

Signed-off-by: Devin Nakamura <devinn@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants