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

Refine range check in JavaCompliance. #264

Closed

Conversation

oraluben
Copy link
Contributor

@oraluben oraluben commented Jun 27, 2022

This PR updates JavaCompliance, including:

  1. Better JavaCompliance.__contains__. It now checks every parts of JavaCompliance instances, rather than only the high and low bounds. mx.JavaCompliance("16") in mx.JavaCompliance("11..15,17") now returns False as expected;
  2. Fix incorrect version check when having requiresConcealed with java compliance specified, this also helps to port Graal to JDK 19.

@oraluben oraluben force-pushed the javacompliance-and-loom-cleanup branch from d1194d6 to f2792b2 Compare June 27, 2022 08:54
@oraluben
Copy link
Contributor Author

@dougxc This is the ready-to-review part of #263, with range tests included.

@oraluben oraluben force-pushed the javacompliance-and-loom-cleanup branch 5 times, most recently from d5118d2 to 6663c03 Compare June 27, 2022 09:10
@oraluben oraluben force-pushed the javacompliance-and-loom-cleanup branch 2 times, most recently from 825a19a to b88c962 Compare June 27, 2022 09:12
@dougxc
Copy link
Member

dougxc commented Jun 27, 2022

@gilles-duboscq could you please help review this as well.

@dougxc
Copy link
Member

dougxc commented Jun 27, 2022

This is a backwards incompatible change. That is, projects that depending on JDKConfig._is_loom will break. Yes, I know that this is a "private" field but I see we have some internal test that use it unfortunately. You may even remember writing them ;-)
I've opened GR-39470 internally to address this but for now, we need to preserve this field and ensure it is initialized properly.

@oraluben
Copy link
Contributor Author

You may even remember writing them ;-)

Now I do 🤣

Anyways, the Loom cleanup is not so urgent at the moment, but I do hope the java compliance changes to be merged if you think it's ok, I'll remove the loom changes from this PR.

@gilles-duboscq
Copy link
Member

I agree we need to keep the loom flags and behaviors, the other changes look good.

@oraluben oraluben force-pushed the javacompliance-and-loom-cleanup branch from b88c962 to a5cae58 Compare June 27, 2022 09:47
@oraluben oraluben changed the title Remove out-of-date Loom logic and refine range check in JavaCompliance. Refine range check in JavaCompliance. Jun 27, 2022
@dougxc
Copy link
Member

dougxc commented Jun 28, 2022

Resolved by 49f40cad895bc2603407f15057b66d0d6d8e1baa.

@dougxc dougxc closed this Jun 28, 2022
@oraluben oraluben deleted the javacompliance-and-loom-cleanup branch June 28, 2022 08:15
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