fix range building for all constructors #151
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
53 tasks
Strum355
force-pushed
the
nsc/constructor-range-fix
branch
from
339d4cb to
8fb4663
Compare
olafurpg
approved these changes
Apr 14, 2021
Contributor
olafurpg
left a comment
olafurpg
left a comment
There was a problem hiding this comment.
LGTM 👍 One non-blocking comment about the fix, feel free to merge.
semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/RangeFinder.java
Outdated
Show resolved
Hide resolved
olafurpg
reviewed
Apr 14, 2021
Contributor
olafurpg
left a comment
olafurpg
left a comment
There was a problem hiding this comment.
The new fix looks a lot cleaner, thank you for following up on that comment 🙏
Contributor
|
I'm not sure what's causing the CI failure. It looks like Gradle is failing sporadically 🤔 Might be worth trying to restart the jobs tomorrow. |
Contributor
Author
|
|
Strum355
modified the milestones:
Code Intelligence Iteration 9,
Code Intelligence Iteration 10
48 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, only generated constructors were delegated to using text search to find the range encompassing the constructor name (aka the class name, which shares the same range as the generated default constructor).
For this reason, non-generated constructors were not hitting the code-path that checks if the
name == "<init>", aka it was a constructor, which followed with logic to build a range based on the owning class's name, causing the range to be built based on the synthetic<init>name.This PR fixes this bug by delegating all constructors to the aforementioned text search codepath. It does it slightly differently by using text search from point, as constructors' preferred/point position starts from the name, avoiding the problem outlined here.
This allows for a minor simplification in the
RangeFinderclass, as we no longer have to prepend the search name with a space. This was only required for constructors which may have the constructor name at the start of the line (after whitespace).