-
Notifications
You must be signed in to change notification settings - Fork 29
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
fixes range deriving for constructs incl classes/interfaces #121
Conversation
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 great! Really nice to have this bug fixed. A few minor comments on style and performance.
I recommend running the benchmarks https://sourcegraph.github.io/lsif-java/docs/benchmarks.html just to validate if the optimizations make a difference. I suspect you won't see a difference in caching the file contents from the compilation unit because we compile virtual files in the benchmark, but the speedup might actually be important for build tools that use a different source file implementation.
semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/RangeFinder.java
Outdated
Show resolved
Hide resolved
semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/RangeFinder.java
Outdated
Show resolved
Hide resolved
emitSymbolOccurrence( | ||
meth.sym, meth, Role.DEFINITION, CompilerRange.FROM_POINT_TO_SYMBOL_NAME); | ||
CompilerRange range = CompilerRange.FROM_POINT_TO_SYMBOL_NAME; | ||
if (meth.params.isEmpty() && (meth.sym.flags() & Flags.GENERATEDCONSTR) != 0L) { |
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.
Minor: might be worth commenting why this is needed here. It looks like we're specifically targeting only constructors? Is there a difference between this and name.contentMatches("<init>")
? Is it possible to reuse the logic between this line and the one in RangeFinder
?
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.
My thinking at the time was that only the no-param constructor would ever be generated, but I realise now this doesn't hold true for the new record
class types, so I'll revisit this line.
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.
We can BTW add Java 15 code to the test suite. Alternatively, we can change the default minimized project to Java 15
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 wouldn't worry too much about Java 15 however, we can deal with it later. I suspect very few users are on Java 15 right now.
JDK 1.8.0_282 This branch:
Current main:
|
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.
Awesome, please ignore the perf comments.
LGTM 👍
Actually, we should still reuse the string for the compilation unit source code because it's unlikely to appear as a bottleneck in our benchmarks. |
semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/RangeFinder.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.
Minor comments
semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/SemanticdbVisitor.java
Outdated
Show resolved
Hide resolved
semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/SemanticdbVisitor.java
Outdated
Show resolved
Hide resolved
semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/SemanticdbVisitor.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 👍 You may need to run fixAll
to please the CI.
This is ready to merge 🚀
Uses text search derived from previous com.sun.source lsif-java (Kotlin edition), which is originally derived from georgewfraser's java language server. Closes #109
Of particular note is that the current changes cause definition occurence to not be emitted. The reference to this no longer existing definition occurrence still gets emitted, we can either follow up with a PR or fix it in this PR.
I feel like moving parts of
SemanticdbVisitor.semanticdbRange(...)
to theRangeFinder
class to keep more of the logic together, but I will revisit this tomorrow.