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.

Sign up for GitHub

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

Jump to bottom

Fix and enable heavy/light value type tests #19716

Merged
merged 2 commits into from
Jun 21, 2024
Merged

Fix and enable heavy/light value type tests #19716

merged 2 commits into from
Jun 21, 2024

Conversation

theresa-m
Copy link
Contributor

  • add back missing error handling for value type inheritance
  • remove stale heavy/light abstract class logic
  • re-enable tests and remove heavy/light naming

Related to #19692

@theresa-m theresa-m added comp:vm comp:test project:valhalla Used to track Project Valhalla related work labels Jun 17, 2024
@theresa-m theresa-m requested a review from hangshao0 June 17, 2024 14:43
@theresa-m theresa-m mentioned this pull request Jun 17, 2024
Merged
@theresa-m
Fix and enable heavy/light ValueTypeTests
3bee06c 
- add back missing error handling for value type inheritance
- remove stale heavy/light abstract class logic
- re-enable tests and remove heavy/light naming

Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
@theresa-m theresa-m requested a review from hangshao0 June 17, 2024 17:15
@theresa-m theresa-m requested a review from hangshao0 June 20, 2024 13:55
@theresa-m theresa-m requested a review from hangshao0 June 20, 2024 19:32
@hangshao0
Copy link
Contributor

Jenkins test sanity,extended alinuxval jdknext

@hangshao0
Copy link
Contributor

jenkins test sanity,extended xmac jdk21

@hangshao0
Copy link
Contributor

Jenkins test sanity,extended zlinux jdk21

@theresa-m
Copy link
Contributor Author

The mac failure looks like a build system issue.

@hangshao0 hangshao0 merged commit fda11be into eclipse-openj9:master Jun 21, 2024
8 of 10 checks passed
keithc-ca
keithc-ca reviewed Jun 21, 2024
View reviewed changes
runtime/vm/createramclass.cpp
Comment on lines -2260 to -2261
* J9ClassHasIdentity is also inherited. If a class cannot be super class
* of value types, its subclasses cannot be super of value types either.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please help me understand why J9ClassHasIdentity should no longer be inherited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In an older version of the spec java.lang.Object was considered to be neither value or identity type. In the current spec it is always considered an identity type, so it is not always correct to inherit this flag since a value class can have java.lang.Object has a superclass.
Since its easy to tell whether this flag should be set or not based on the ACC_IDENTITY flag I thought it made more sense to check for correct inheritance where J9ClassIsValueType is set.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.

@theresa-m theresa-m deleted the vttests_7 branch October 11, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keithc-ca keithc-ca keithc-ca left review comments

@hangshao0 hangshao0 hangshao0 approved these changes

Assignees
No one assigned
Labels
comp:test comp:vm project:valhalla Used to track Project Valhalla related work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@theresa-m @hangshao0 @keithc-ca