-
-
Notifications
You must be signed in to change notification settings - Fork 388
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
Optimize semantic token extraction logic #4050
Optimize semantic token extraction logic #4050
Conversation
rope :: !Rope -- the remains of rope we are working on | ||
, cursor :: !Char.Position -- the cursor position of the current rope to the start of the original file in code point position | ||
, columnsInUtf16 :: !UInt -- the column of the start of the current rope in utf16 | ||
, rangeHsSemanticList :: [(Range, HsSemanticTokenType)] -- (range, token type) in reverse order |
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.
can we move this out the state? It seems like all the other Tokenizer
functions return ()
, instead we could just return [(Range, HsSemanticTokenType)]
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.
Sure, maybe we can do a CPS to collect them one by one or switch to a structure with better concatenation time complexity.
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.
DList fit into it. I've switch to Dlist and remove rangeHsSemanticList out the state
cc @wz1000 |
Yes. I'm ambivalent on the |
Just curious, I wonder why |
Now that I see it, I wonder if maybe the previous approach was cleaner. But it is totally up to you! |
I see, I review it a bit, both should have simliar cleaness(DList cut a few lines), DList seems make the type look better. I'll stick with DList, seems more explicit in the type level. |
The following idea comes from wz1000.
A follow up of #3958 , we have added a tokenizor to walk the hieAst along with the file rope, it means we no longer need to do the detour of storing temperal result as
Map Range (Set identifier)
, instead we can optimize by fusing most of the logic into tokenizer and return[(Range, HsSemanticTokenType)]
directly.