Implement Eq and Hash for GreenTokenData - #174
Open
hurryabit wants to merge 22 commits into
Open
Conversation
Use niching for Checkpoint
Both of these use source code locations to identify the node, which can get invalidated by syntax tree mutations. This commit adds assertions to prevent their use, and adds documentation to inform users of the issue Fixes rust-analyzer#150
`std::mem::offset` has been availible on stable since 1.77.0. `memoffset` uses this when compiled against a recent enough rustc. Moving to the implementation in `std` lets us drop both `memoffset` and `autocfg` from our dependencies.
Also, rephrase slightly.
Fix typo in docstring: two => to
Remove needless .into()
This allows users of the API to apply a filter on the SyntaxKind before materializing concrete SyntaxNode/Token objects, which require a memory allocation for the NodeData. For slint, this removes ~400k allocations when parsing a largish project, about 50% of all rowan allocations (850k down to 450k).
When possible, reuse the allocated NodeData instead of allocating a new one for each iteration. This can be done as long as the refcount is 1 - we can then just rewire the values in NodeData to point to the new item. This removes ~220k allocations when compiling a largish slint file, about half of all rowan allocations that happen during iteration, i.e. we go from 450k down to 230k.
This makes this API actually useful from the outside - there is no lifetime problem with the matcher callback, and we can remap the raw Kind to the Language::Kind on the fly.
…teration Optimize children iteration by reusing NodeData if possible
Fixes an assertion in debug builds which I accidentally introduced when attending the review comments for [1] in [2] - instead of only removing the increment, I also removed the decrement which was wrong - `std::mem::forget` only allows us to remove the increment, but the decrement is still needed before free since we are in a place of code that is by definition only run when the rc value is set to 1. See also `can_take_ptr`. I did not spot this earlier since I ran the integration test on a release build, where the assertion was disabled. It's sad that the rowan repo itself doesn't have any big test coverage in this repo itself, but rather relies on external repos for testing purposes... [1]: rust-analyzer#171 (comment) [2]: https://github.com/rust-analyzer/rowan/compare/60a632ad984ab451e32058169193511154c675a9..ab5463e2749330be6846886c21e98c83caca8598 Fixes: rust-analyzer#172
Decrement refcount before calling free in to_next_sibling
`GreenToken` implements all traits required to use it as the key type of a hash map. It also implements `Borrow<GreenTokenData>`. Thus is would be nice if `GreenTokenData` also implements `Eq` and `Hash` such that the result of `SyntaxToken::green` can be used for lookups in such a hash map without additional overhead in terms of sytntax and runtime.
Contributor
|
Consider if you might want to intern GreenTokens&Nodes, and use pointer-equality and identity instead of content hashing |
Contributor
Author
|
Thanks for your quick response. That's basically what I'm doing. I'm still prototyping and wanted to use the I just realised that the |
|
What is the status of this? It received a review, but the necessary action isn't clear to me. |
Contributor
Author
|
I found a different way to solve my problem (I don't remember how though) and have given up on this PR. Shall I close it or would you be interested in this feature? |
|
I don't need it. Closing it is probably fine. The general advise is to implement standard traits such as |
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
13 participants
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.
GreenTokenimplements all traits required to use it as the key type of a hash map. It also implementsBorrow<GreenTokenData>. Thus is would be nice ifGreenTokenDataalso implementedEqandHashsuch that the result ofSyntaxToken::greencould be used for lookups in such a hash map without additional overhead in terms of syntax or runtime.