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

Check NullRestricted attribute #18179

Merged

Conversation

a7ehuo
Copy link
Contributor

@a7ehuo a7ehuo commented Sep 21, 2023

NullRestricted field attribute is introduced in JEP 401.

  • Create API to check NullRestricted
  • Replace calls to isFieldQType with isFieldNullRestricted
  • Remove isClassRefPrimitiveValueType
  • Update the test to test JIT'd methods

Related: #18170

@a7ehuo
Copy link
Contributor Author

a7ehuo commented Sep 21, 2023

@hzongaro May I ask you to review this change? Thanks!
@theresa-m May I ask you to review the change under Valhalla test? Thanks!

Copy link
Contributor

@theresa-m theresa-m left a comment

Choose a reason for hiding this comment

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

I have reviewed the test code.

Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

Thanks, Annabelle! Just a few comments.

@a7ehuo a7ehuo force-pushed the check-isFieldNullRestricted-PR branch from 8cb6cb5 to 32f6437 Compare September 22, 2023 22:41
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Sep 22, 2023

@hzongaro @theresa-m All comments are addressed. Ready for another review. Thanks!

Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. It looks like there are still a few more cases where _methodSymbol->isStatic() was being used in the old code where it shouldn't have been.

`NullRestricted` field attribute is introduced in JEP 401.
- Create API to check `NullRestricted`
- Replace calls to `isFieldQType` with `isFieldNullRestricted`
- Remove `isClassRefPrimitiveValueType`
- Update the test to test JIT'd methods

Related: eclipse-openj9#18170

Signed-off-by: Annabelle Huo <Annabelle.Huo@ibm.com>
@a7ehuo a7ehuo force-pushed the check-isFieldNullRestricted-PR branch from 32f6437 to 0405834 Compare September 25, 2023 15:03
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Sep 25, 2023

@hzongaro The latest comments are addressed in 0405834. Ready for another review. Thanks!

Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks.

@hzongaro
Copy link
Member

Jenkins test sanity xlinuxval,xlinuxvalst jdknext

@hzongaro hzongaro merged commit 20fb92b into eclipse-openj9:master Sep 26, 2023
@a7ehuo a7ehuo deleted the check-isFieldNullRestricted-PR branch March 6, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants