-
Notifications
You must be signed in to change notification settings - Fork 30
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
JDK 9+ Support #91
JDK 9+ Support #91
Conversation
@@ -1,4 +1,5 @@ | |||
package minimized; | |||
^^^^^ reference minimized/Enums# |
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.
This one is unusual 🤔 @olafurpg
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.
🤔 Hmmm not sure what's going on here. It's not a blocking issue. I think it makes sense that we try to document these quirks at some later point as issues. For now, it's most important to the the infrastructure working.
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.
The diffs look like a mix of nice improvements and minor regressions. I think it's OK to merge these changes and follow up with a PR that adds new testing infrastructure to compile with java 8 and java 11 within the same sbt build.
Some blocking issues:
- Update CI configuration to use java 11.
+++ .github/workflows/ci.yml
- uses: olafurpg/setup-scala@v10
+ with:
+ java-version: [email protected]
- Update readme to reflect Java 11 support.
- Update title and description of the PR.The format I try to follow in PR description is something like "Previously, the semanticdb-javac compiler plugin only supported Java 8. The plugin crashed when compiling with Java 11. This PR adds support for Java 11 so that the plugin works on both Java 8 and Java 11.".
- Manually confirm that the plugin work on Java 8. We can fix the testing infrastructure later to automatically prevent regressions.
| `snapshot/testOnly tests.LibrarySnapshotSuite` | sbt | Runs slow snapshot tests. Indexes a corpus of external Java libraries. | | ||
| `snapshot/test` | sbt | Runs all snapshot tests. | | ||
| `snapshot/run` | sbt | Update snapshot tests. Use this command after you have fixed a bug. | | ||
| `snapshots/testOnly tests.MinimizedSnapshotSuite` | sbt | Runs fast snapshot tests. Indexes a small set of files under `tests/minimized`. | |
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.
👍
@@ -148,6 +149,7 @@ public static Handler createHandler(Looper looper, boolean async) { | |||
} | |||
|
|||
return new Handler(looper); | |||
// ^^^^^^^^^^^^^^^^^^^ reference `<init>`# |
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.
This looks like an improvement 👍
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.
Im assuming this isnt correct yet, but progress?
public boolean areItemsTheSame(EpoxyModel<?> oldItem, EpoxyModel<?> newItem) { | ||
// ^^^^^^^^^^^^^^^ definition com/airbnb/epoxy/EpoxyControllerAdapter#ITEM_CALLBACK.``#areItemsTheSame(). |
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.
Improvement 👍
@@ -1,4 +1,5 @@ | |||
package minimized; | |||
^^^^^ reference minimized/Enums# |
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.
🤔 Hmmm not sure what's going on here. It's not a blocking issue. I think it makes sense that we try to document these quirks at some later point as issues. For now, it's most important to the the infrastructure working.
while (lookup != null) { | ||
if (lookup.sym != null) { | ||
peers.add(lookup.sym); | ||
Iterable<? extends Element> elements = sym.owner.getEnclosedElements(); |
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.
Nice find 👍 This looks like a decent replacement for members().lookup()
public boolean isNone(Symbol sym) { | ||
return sym == null || sym.kind == Kinds.NIL || (sym.kind & Kinds.ERRONEOUS) != 0; | ||
return sym == null; // || sym.kind == Kinds.Kind.NIL || sym.kind == Kinds.Kind.ERR; |
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.
This seems to be working ok. No? I'm OK with removing the comment.
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.
The commented part is not backwards compatible. JDK 8 it was Kinds.NIL
etc, JDK 9+ the enum was moved to be a nested enum. The behaviour appears to be identical at best, but I'm unsure if we're testing enough edge cases to surface where this change may not be sufficient
semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/GlobalSymbolsCache.java
Outdated
Show resolved
Hide resolved
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.
LGTM 🚀
Previously, the semanticdb-javac compiler plugin only supported Java 8. The plugin failed when compiling with Java 9+ due to our use of 'unsupported' APIs that changed from JDK 9.
This PR adds support for all JDKs from 9 onwards in addition to the previously supported JDK 8 by
Notes:
Closes #90