feat: range metadata#131
Closed
doorgan wants to merge 1 commit intoelixir-tools:mainfrom
Closed
Conversation
Contributor
Author
|
Despite what profiling initially shows, it does not make a significant difference indexing real projects in expert |
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
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.
Adds a
:rangeproperty to the nodes metadataThe performance impact of this is detailed in expert-lsp/expert#651
TL;DR I expect this to help quite a bit when indexing large codebases in Expert, but I think I'm getting limited by IO in that project so I can't be 100% confident on that. Profiling shows significant improvements though, but it's not the practical reality I experience.
I decided on this format for the range metadata:
%{start: {line, column}, end: {line, column}. The rationale is that keyword ists are really slow to read in a very hot path, while maps have better random access. line/column pairs are tuples for the same reason.{start_line, start_column, end_line, end_column}didn't provide much of an improvement over maps in what I could measure and the ergonomics were terrible(no nice way to get only the start or only the end of a node).This also adds a new dedicated ranges test file which is mostly a translation of the Sourceror ranges test suite to verify parity/precision. The diff on the spitfire tests is mostly updating assertions to ignore the Spitfire specific metadata when comparing against Elixir's own AST