Skip to content
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

Merged
merged 1 commit into from
Jan 5, 2015

Conversation

bfops
Copy link
Contributor

@bfops bfops commented Dec 23, 2014

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 clones the key even when it's already owned.
  • With small keys (e.g. ints), taking a reference seems wasteful.

r? @gankro
cc: @cgaebel

@rust-highfive
Copy link
Collaborator

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.

@Gankra
Copy link
Contributor

Gankra commented Dec 23, 2014

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.

@Gankra
Copy link
Contributor

Gankra commented Dec 23, 2014

Do you intend to do the other collections/TODOs in this PR?

@bfops
Copy link
Contributor Author

bfops commented Dec 23, 2014

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 Entry with O instead of K though?

@cgaebel
Copy link
Contributor

cgaebel commented Dec 23, 2014

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
notifications@github.com wrote:

Do you intend to do the other collections/TODOs in this PR?

Reply to this email directly or view it on GitHub:
#20163 (comment)

@@ -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(),
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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 Ks, not Qs). 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.

@bfops bfops changed the title Upgrade HashMap to new entry API in RFC 509 (https://github.com/rust-lang/rust/issues/19986) Upgrade to new entry API in RFC 509 (https://github.com/rust-lang/rust/issues/19986) Dec 23, 2014
/// 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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entry.into_mut()?

Copy link
Contributor Author

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.

@bfops
Copy link
Contributor Author

bfops commented Dec 24, 2014

Updated so that to_owned() is only called once VacantEntry::insert is called. For some reason, the tests won't build (make check-stage1-std -j10?), but make finishes fine.

/home/ben/devl/rust/rust/src/libstd/collections/hash/map.rs:2188:17: 2188:27 error: type `collections::hash::map::VacantEntry<'_, str, collections::string::String, int>` does not implement any method in scope named `insert`
/home/ben/devl/rust/rust/src/libstd/collections/hash/map.rs:2188           entry.insert(30);

@bfops
Copy link
Contributor Author

bfops commented Dec 25, 2014

BTreeMap converted, but the tests are still failing to build with the weird message that insert isn't defined for VacantEntry.. any ideas?

Edit: *HashMap's tests are still failing. BTreeMap's probably are too.

@cgaebel
Copy link
Contributor

cgaebel commented Dec 25, 2014

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 fn insert there!

@bfops
Copy link
Contributor Author

bfops commented Dec 25, 2014

@cgaebel *for HashMap. See the errors from the post above. For some reason, it doesn't agree that (HashMap's) VacantEntry has insert defined..

@cgaebel
Copy link
Contributor

cgaebel commented Dec 25, 2014

BTreeMap shouldn't be using HashMap for anything.

@bfops
Copy link
Contributor Author

bfops commented Dec 25, 2014

Tests are still having issues, but I think I'm done otherwise? Help figuring out these errors would be appreciated. Adding use super::VacantEntry; doesn't help.

rustc: x86_64-unknown-linux-gnu/stage1/test/collectionstest-x86_64-unknown-linux-gnu
rustc: x86_64-unknown-linux-gnu/stage1/test/stdtest-x86_64-unknown-linux-gnu
/home/ben/devl/rust/rust/src/libcollections/btree/map.rs:1576:17: 1576:27 error: type `btree::map::VacantEntry<'_, str, string::String, int>` does not implement any method in scope named `insert`
/home/ben/devl/rust/rust/src/libcollections/btree/map.rs:1576           entry.insert(30);
                                                                              ^~~~~~~~~~
error: aborting due to previous error
/home/ben/devl/rust/rust/mk/tests.mk:409: recipe for target 'x86_64-unknown-linux-gnu/stage1/test/collectionstest-x86_64-unknown-linux-gnu' failed
make: *** [x86_64-unknown-linux-gnu/stage1/test/collectionstest-x86_64-unknown-linux-gnu] Error 101
make: *** Waiting for unfinished jobs....
/home/ben/devl/rust/rust/src/libstd/collections/hash/map.rs:2192:17: 2192:27 error: type `collections::hash::map::VacantEntry<'_, str, collections::string::String, int>` does not implement any method in scope named `insert`
/home/ben/devl/rust/rust/src/libstd/collections/hash/map.rs:2192           entry.insert(30);
                                                                                 ^~~~~~~~~~

@bfops
Copy link
Contributor Author

bfops commented Dec 26, 2014

@gankro PTAL? I think I got all the TODOs. make and make check-stage1-{collections,std} -j10 succeed; I'm running make check -j10 right now.

@bfops
Copy link
Contributor Author

bfops commented Dec 26, 2014

If everything looks good and make check passes, I'll squash the commits and deal with the merge conflicts.

@bfops
Copy link
Contributor Author

bfops commented Dec 26, 2014

Success! test result: ok. 1712 passed; 0 failed; 68 ignored; 0 measured

@Gankra
Copy link
Contributor

Gankra commented Dec 30, 2014

Needs rebase :(


impl BorrowFrom<int> for UnownedInt {
fn borrow_from(k: &int) -> &UnownedInt {
unsafe { ::std::mem::transmute(k) }
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, unused!

@Gankra
Copy link
Contributor

Gankra commented Dec 31, 2014

@bfops can you also mark all Entry related structs/enums/methods as #[stable], with the exception of the "new" get method (which should be unstable with the standard collection "dust to settle" message)? I specifically avoided it to avoid conflicting with this.

@bfops
Copy link
Contributor Author

bfops commented Jan 2, 2015

@gankro Alright I think I got everything. Is it alright if I squash things? I think the outstanding questions are whether those &str tests are worth having, and whether unwrap_or_else should have a verbose parameter name like vacant_entry for the closure.

@Gankra
Copy link
Contributor

Gankra commented Jan 2, 2015

Eh, leave them verbose; someone can change it later if they don't like it.

@Gankra
Copy link
Contributor

Gankra commented Jan 2, 2015

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]

@Gankra
Copy link
Contributor

Gankra commented Jan 4, 2015

ack something big just landed, needs a rebase :(

@bfops
Copy link
Contributor Author

bfops commented Jan 4, 2015

Rebased.. tests are failing, but it looks like they're failing without this change too..?

@bfops
Copy link
Contributor Author

bfops commented Jan 4, 2015

Ach needs another rebase xP.

@bfops
Copy link
Contributor Author

bfops commented Jan 4, 2015

Alright everything seems to work! Everything passes once I unset RUST_BACKTRACE.

bors added a commit that referenced this pull request Jan 5, 2015
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
@bors bors merged commit 400c3a0 into rust-lang:master Jan 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants