-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Upgrade to new entry API in RFC 509 (https://github.com/rust-lang/rust/issues/19986) #20163
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gankro (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see CONTRIBUTING.md for more information. |
I sympathize with the actually-owned case being wasteful, although Rust is already pretty heavily scattered with "I hope LLVM can optimize this away..." in that regards. I hope one day we'll have the appropriate abstraction to fully handle owned and non-owned data alike. We discussed this a lot while writing these collections RFCs. I've forgotten the precise details but basically every solution we had wasn't quite perfect. We settled on this as a decent "lowest common denominator" that should be fairly-cleanly upgradeable to the "ideal" API, whatever that might be. |
Do you intend to do the other collections/TODOs in this PR? |
I mainly just wanted this out to make sure that nothing looks (horribly) wrong. I can definitely tack on the other TODOs. I was wondering why we want |
Llvm won't optimize away mallocs unless it can prove they aren't used, which it can't in this case. On Mon, Dec 22, 2014 at 11:04 PM, Alexis Beingessner
|
@@ -1198,7 +1199,7 @@ fn search_entry_hashed<'a, K: Eq, V>(table: &'a mut RawTable<K,V>, hash: SafeHas | |||
// Found a hole! | |||
return Vacant(VacantEntry { | |||
hash: hash, | |||
key: k, | |||
key: k.to_owned(), |
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.
You should be moving the k
value into the Entry; that way it only needs to be to_owned if they decide to actually insert
.
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.
That's why the entry is O
and not K
(in retrospect, I guess it also needs the K
for the Map itself). It might make sense to replace O
with Q
to match the other HashMap methods, though?
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.
Oh right, of course. Silly me.
Is it okay for Entry
to be Entry<'a, Q:'a, K:'a, V:'a>
? The Entry
has references to the underlying table (which contains K
s, not Q
s). It might be possible to make it work with just Q
, but I think it would mean pushing all the ToOwned
stuff into all parts of hashtables and hashmaps.
/// Returns a mutable reference to the entry if occupied, or the VacantEntry if vacant | ||
pub fn get(self) -> Result<&'a mut V, VacantEntry<'a, K, V>> { | ||
match self { | ||
Occupied(entry) => Ok(entry.elem.into_mut_refs().1), |
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.
entry.into_mut()
?
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.
Done! There were lifetime issues last time I tried (which is why I plugged in the definition). I'm not sure what changed.
Updated so that
|
Edit: * |
it isn't defined for VacantEntry in BTreeMap: impl<'a, Q: ToOwned<K>, K: Ord, V> VacantEntry<'a, Q, K, V> {
/// Sets the value of the entry with the VacantEntry's key,
/// and returns a mutable reference to it.
pub fn set(self, value: V) -> &'a mut V {
self.stack.insert(self.key.to_owned(), value)
}
} No |
@cgaebel *for |
BTreeMap shouldn't be using HashMap for anything. |
Tests are still having issues, but I think I'm done otherwise? Help figuring out these errors would be appreciated. Adding
|
@gankro PTAL? I think I got all the TODOs. |
If everything looks good and |
Success! |
Needs rebase :( |
|
||
impl BorrowFrom<int> for UnownedInt { | ||
fn borrow_from(k: &int) -> &UnownedInt { | ||
unsafe { ::std::mem::transmute(k) } |
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.
I don't understand the point of all this?
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.
Oops, unused!
@bfops can you also mark all Entry related structs/enums/methods as |
@gankro Alright I think I got everything. Is it alright if I squash things? I think the outstanding questions are whether those |
Eh, leave them verbose; someone can change it later if they don't like it. |
So yeah, remove the "extra" tests, ad feel free to squash. Also make sure you specify in the relevant commits that this is a [breaking-change] |
ack something big just landed, needs a rebase :( |
Rebased.. tests are failing, but it looks like they're failing without this change too..? |
Ach needs another rebase xP. |
Alright everything seems to work! Everything passes once I |
TODOs: - ~~Entry is still `<'a, K, V>` instead of `<'a, O, V>`~~ - ~~BTreeMap is still outstanding~~. - ~~Transform appropriate things into `.entry(...).get().or_else(|e| ...)`~~ Things that make me frowny face: - I'm not happy about the fact that this `clone`s the key even when it's already owned. - With small keys (e.g. `int`s), taking a reference seems wasteful. r? @gankro cc: @cgaebel
TODOs:
Entry is still<'a, K, V>
instead of<'a, O, V>
BTreeMap is still outstanding.Transform appropriate things into.entry(...).get().or_else(|e| ...)
Things that make me frowny face:
clone
s the key even when it's already owned.int
s), taking a reference seems wasteful.r? @gankro
cc: @cgaebel