Add a splay tree implementation #19
Conversation
| // need to get visited. I don't believe that it's safe to allow lookups | ||
| // while the tree is being iterated. Right now there are no iterators | ||
| // exposed on this splay tree implementation, and more thought would be | ||
| // required if there were. |
There was a problem hiding this comment.
Isn't this still UB without an UnsafeCell somewhere? I guess this is just another face of rust-lang/rust#19733
That is, you can get a ref x to a Node, someone can call get on the tree, this changes the ptrs on the Node. Then you try to follow one of those ptrs through x. What happens?
There was a problem hiding this comment.
Hm, it probably is! This was all written before we even had UnsafeCell :)
Updated to use UnsafeCell for the root node now.
There was a problem hiding this comment.
Does that really just work? What does UnsafeCell actually do? Any thoughts on the issue I linked? I'm honestly lost on what is even UB anymore 😞
There was a problem hiding this comment.
I can't say I've personally taken the time to think this through too closely with respect to UB. That being said, correctly informing the compiler is important for other compiler pieces such as driving what can be inside of a const. Additionally, if I had correctly used UnsafeCell then the compiler would automatically determine that SplayTree is NoSync (which it definitely isn't!). Which is to say that for now UnsafeCell is unambiguously correct for reasons other than mutable aliasing, and I'd have to think more about whether it was actual UB before.
|
Cool! This is like the first thing I tried to implement in Rust, and it led me down some serious safety/ownership rabbit holes that I was never able to really resolve fully. Interested to see how you address this! |
|
My impl for posterity: https://github.com/Gankro/rust-datastructures/blob/master/src/splaytree.rs |
|
|
||
| /// Moves all values out of this map, transferring ownership to the given | ||
| /// iterator. | ||
| pub fn into_iter(&mut self) -> IntoIter<K, V> { |
There was a problem hiding this comment.
This should probably take self even though it works with &mut self since it's basically a logic error to call into_iter twice.
I've had a [splay tree implementation][gh] externally, but it seems like this may be a good new home for it! [gh]: https://github.com/alexcrichton/splay-rs This splay tree implementation is not quite of the quality one would expect of a standard collection because it primarily doesn't implement iteration by reference. Due to the self-modifying nature of lookups, I'm not 100% sure how to safely implement iteration at this time, so only consuming iteration is provided. Otherwise though I've tried to mirror the interface of HashMap as much as possible in the implementation! The implementation also leverages very little unsafe code, so it's not the most efficient implementation by a long shot. Primarily the splay() method is probably quite a bit larger than it needs to be!
|
For obvious reasons this is pretty low priority for me. I'll try to review it some time when I find the time. |
|
No worries! I just figured it may be a good place to put it, if it causes any level of stress it's probably not worth it :) |
|
At very least I'm sure there's some cool ideas in here. Or, at worst, something to poke fun at you with. 😈 I'm just very heavily focused on experimenting with general concepts and APIs, rather than concrete collections -- for now. |
|
I'll work on this elsewhere for now, perhaps later! |
I've had a splay tree implementation externally, but it seems like this
may be a good new home for it!
This splay tree implementation is not quite of the quality one would expect of a
standard collection because it primarily doesn't implement iteration by
reference. Due to the self-modifying nature of lookups, I'm not 100% sure how to
safely implement iteration at this time, so only consuming iteration is
provided. Otherwise though I've tried to mirror the interface of HashMap as much
as possible in the implementation!
The implementation also leverages very little unsafe code, so it's not the most
efficient implementation by a long shot. Primarily the splay() method is
probably quite a bit larger than it needs to be!