Skip to content

Check NullRestricted attribute - #18179

Merged
hzongaro merged 1 commit into
eclipse-openj9:masterfrom
a7ehuo:check-isFieldNullRestricted-PR
Sep 26, 2023
Merged

Check NullRestricted attribute#18179
hzongaro merged 1 commit into
eclipse-openj9:masterfrom
a7ehuo:check-isFieldNullRestricted-PR

Conversation

@a7ehuo

@a7ehuo a7ehuo commented Sep 21, 2023

Copy link
Copy Markdown
Contributor

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

a7ehuo commented Sep 21, 2023

Copy link
Copy Markdown
Contributor Author

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

@theresa-m theresa-m left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread test/functional/Valhalla/src/org/openj9/test/lworld/ValhallaAttributeTests.java Outdated
Comment thread test/functional/Valhalla/playlist.xml Outdated

@hzongaro hzongaro left a comment

Copy link
Copy Markdown
Member

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.

Comment thread runtime/compiler/net/CommunicationStream.hpp Outdated
Comment thread runtime/compiler/ilgen/Walker.cpp Outdated
Comment thread runtime/compiler/ilgen/Walker.cpp Outdated
Comment thread runtime/compiler/ilgen/Walker.cpp Outdated
Comment thread runtime/compiler/env/j9method.cpp Outdated
Comment thread runtime/compiler/env/j9method.h Outdated
Comment thread runtime/compiler/env/j9method.h Outdated
Comment thread runtime/compiler/ilgen/Walker.cpp Outdated
@a7ehuo
a7ehuo force-pushed the check-isFieldNullRestricted-PR branch from 8cb6cb5 to 32f6437 Compare September 22, 2023 22:41
@a7ehuo

a7ehuo commented Sep 22, 2023

Copy link
Copy Markdown
Contributor Author

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

@hzongaro hzongaro left a comment

Copy link
Copy Markdown
Member

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.

Comment thread runtime/compiler/env/j9method.cpp Outdated
Comment thread runtime/compiler/ilgen/Walker.cpp Outdated
Comment thread runtime/compiler/ilgen/Walker.cpp Outdated
Comment thread runtime/compiler/ilgen/Walker.cpp Outdated
`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

a7ehuo commented Sep 25, 2023

Copy link
Copy Markdown
Contributor Author

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

@hzongaro hzongaro left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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