Skip to content

Implement append and split_off for VecMap (RFC 509) #22494

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 23, 2015
Merged

Implement append and split_off for VecMap (RFC 509) #22494

merged 1 commit into from
Feb 23, 2015

Conversation

msiemens
Copy link
Contributor

Implements append() and split_off() for VecMap. It's worth noting that append() will overwrite existing keys (the RFC doesn't specify how append() should handle duplicate keys).

cc #19986

r? @gankro

Gankra
Gankra reviewed Feb 18, 2015
View reviewed changes
src/libcollections/vec_map.rs
/// assert_eq!(b[4], "d");
/// ```
pub fn split_off(&mut self, at: usize) -> Self {
assert!(at <= self.len(), "`at` out of bounds");
Copy link
Contributor

Choose a reason for hiding this comment

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

For Sorted Maps, at shouldn't have any assert, I reckon. It's basically a predecessor query.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is, the backing storage is an implementation detail for Maps, unlike sequences.

@Gankra
Copy link
Contributor

Gankra commented Feb 18, 2015

I'm not sure exactly how much VecMap should try to help out with foot-guns. In particular should it assume that inputs are reasonable and that VecMaps are small and dense? Or should it make a best effort not to do something catastrophic when the VecMap has a lot of empty space (particularly at the tail).

CC @huonw

Gankra
Gankra reviewed Feb 21, 2015
View reviewed changes
src/libcollections/vec_map.rs

if at == 0 {
// Move all elements to other
other.append(self);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems inefficient. Why not just mem::swap(self, other)?

@Gankra
Copy link
Contributor

Gankra commented Feb 21, 2015

r=me with the last two changes and squashed commits. 😄

@msiemens msiemens force-pushed the vec_map-append-split_off branch from 55f0e06 to 87e1bbb Compare February 21, 2015 10:49
@msiemens
Copy link
Contributor Author

I've addressed both comments and squashed the commits. I think, we're ready to go 🎉 .
Thanks for reviewing, @gankro, I'm learning a lot from your feedback!

@msiemens
Copy link
Contributor Author

By the way, shouldn't I also include a #[unstable(...)] tag for both methods?

@msiemens msiemens force-pushed the vec_map-append-split_off branch from 87e1bbb to 5d8d879 Compare February 21, 2015 11:24
@Gankra
Copy link
Contributor

Gankra commented Feb 21, 2015

Oh, whoops, yes! I am still getting used to the new stability stuff.

@msiemens msiemens force-pushed the vec_map-append-split_off branch 2 times, most recently from 3ee0028 to 0b2029d Compare February 21, 2015 14:54
@msiemens
Copy link
Contributor Author

I've added them now. BTW: Is there a convention when to use #[inline]?

@Gankra
Copy link
Contributor

Gankra commented Feb 21, 2015

Ugh #[inline] is basically cargo culting in the standard libraries IMHO. It does two things: It hints to LLVM that inlining is more desriable than usual, and it makes the function a candidate for cross-crate-inling (by including the source in the compiled crate).

The first one is very dubious, because LLVM's defaults should be smarter than us in this regard. The second one is completely inapplicable for almost all collections code because generic code is already available for cross-crate inlining (since you need the source for monomorphization).

It's a battle I simply don't have the energy to fight. Everyone does it. It'd be great if you excluded them IMO, but I can't fight the world on this.

@msiemens msiemens force-pushed the vec_map-append-split_off branch from 0b2029d to ec7145e Compare February 21, 2015 15:03
@msiemens
Copy link
Contributor Author

It'd be great if you excluded them IMO, but I can't fight the world on this.

Alright, I've removed them. I always thought that the compiler should know better what to inline. Didn't know about the cross-crate-inlining, though.

@Gankra
Copy link
Contributor

Gankra commented Feb 21, 2015

@bors r+ ec7145e

Great work!

@bors
Copy link
Collaborator

bors commented Feb 22, 2015

⌛ Testing commit ec7145e with merge 81cec05...

@bors
Copy link
Collaborator

bors commented Feb 22, 2015

💔 Test failed - auto-mac-64-opt

@Manishearth
Copy link
Member 

/Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-opt/build/src/libcollections/vec_map.rs:364:5: 366:6 error: method is never used: `append`, #[deny(dead_code)] on by default
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-opt/build/src/libcollections/vec_map.rs:364     fn append(&mut self, other: &mut Self) {
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-opt/build/src/libcollections/vec_map.rs:365         self.extend(other.drain());
/Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-opt/build/src/libcollections/vec_map.rs:366     }

@Gankra
Copy link
Contributor

Gankra commented Feb 22, 2015

Whoops, needs a pub.

@msiemens
Implement append and split_off for VecMap (RFC 509)
25d3e01 
Implements `append()` and `split_off()` for `VecMap`. It's worth noting
that `append()` will overwrite existing keys (the RFC doesn't specify how
`append()` should handle duplicate keys).

cc rust-lang#19986
@msiemens msiemens force-pushed the vec_map-append-split_off branch from ec7145e to 25d3e01 Compare February 22, 2015 12:42
@msiemens
Copy link
Contributor Author

Oops! Fixed

@Manishearth
Copy link
Member

@bors: r=Gankro 25d3e01

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 23, 2015
@Manishearth @alexcrichton
Rollup merge of rust-lang#22494 - msiemens:vec_map-append-split_off, …
3b4b901 
…r=Gankro
@alexcrichton alexcrichton merged commit 25d3e01 into rust-lang:master Feb 23, 2015
This was referenced Apr 27, 2015
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Gankra Gankra

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@msiemens @Gankra @bors @Manishearth @alexcrichton