-
Notifications
You must be signed in to change notification settings - Fork 70
fix: support JDK 25 API change #1305
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
Conversation
|
@crogoz @felixdesouza @CRogers @FinlayRJW anything I can help you with ? |
|
@gnodet similar fix was also in google-java-format |
Ah, didn't check that one. Both should be fixed I suppose. |
|
They fixed it with a static variable, that may perform better |
|
The problem here is more that the team is not responsive at all... |
| try { | ||
| // Fall back to pre-JDK 25 method (Queue<JCDiagnostic>) | ||
| Method queueMethod = DeferredDiagnosticHandler.class.getMethod("getDiagnostics"); | ||
| return (Collection<JCDiagnostic>) queueMethod.invoke(handler); | ||
| } catch (Exception e2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this particular change does anything? There is no difference between this and lines 480-482. It makes more sense to do the same/equivalent change as google-java-format: https://github.com/google/google-java-format/pull/1247/files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixdesouza You're right, I removed the second fallback which is completely useless.
I also cached the method handle in a static final field to get closer to google/google-java-format#1247. Just cherry-picking their commit could work too I suppose.
|
Any chance of seeing this merged/released soon? I'm starting to test our builds under J25ea and this was the first thing I hit. |
|
I've made another PR to apply this change at #1367 - just because some our changelog infra doesn't work well on forks (there's a fix in the works for it but not ready yet) |
|
Released as 2.71.0 |
Before this PR
The build fails with the following error when running on JDK 25
After this PR
The build works correctly
Possible downsides?
The commit uses reflection to call the correct method, but this is the only way to support the incompatible API change introduced in JDK 25 while still supporting older JDK.