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

implement Entry API for SmallIntMap #17879

Closed
wants to merge 1 commit into from

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Oct 9, 2014

The contents of this PR make me legit mad. I really hope that I'm just completely stupid tonight, and there's a way better way to do this. It doesn't help that this API is basically pointless to provide on SmallIntMap. But we've gotta provide a uniform interface, so...

yeah

😡

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

pub fn entry<'a>(&'a mut self, key: uint) -> Entry<'a, V> {
let self_ptr = self as *mut _;
match self.find_mut(&key) {
// FIXME: So, this is absolutely awful. We absolutely *do not* need a raw ptr for
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried something like:

{
    match self.find_mut(&key) {
        Some(val) => return Occupied(...)
        None => {}
    }
}
Vacant(...)

EDIT: It doesn't work. I'm surprised the borrow "bleeds" out of the enclosing block.
EDIT EDIT: I think it's because the region of the returned expression becomes linked to that of the return value of the function, which is the same as that of self ('a), which is linked to the region of the function body. This ends up extending the lifetime. Come to think of it, I think I've seen people complain about this before.

@bkoropoff
Copy link
Contributor

Your borrow checker pain seems to be issue #6393. The problem attempting to use return as a workaround is called out as a particularly tricky case.

val: &'a mut V,
// We only need this for `take`. Should never be null, and we'll only use it when
// we we'll never touch `val` again. Totally safe, just lame that we need this.
// The alternative would be doing the indexing on every op, which would make this API
Copy link
Contributor

Choose a reason for hiding this comment

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

Another alternative would be storing &'a mut Option<V>, which would be totally safe but would require unwrapping the Option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still need the ref to decrement len :(

Copy link
Contributor

Choose a reason for hiding this comment

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

What len is being decremented? SmallIntMap doesn't cache the number of set elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gereeter Oh! You know, I just totally assumed. That makes the Option solution much more viable, though the unwraps are unsavoury.

@pcwalton
Copy link
Contributor

r? @aturon

@aturon
Copy link
Member

aturon commented Oct 27, 2014

I spent a few minutes trying to find a better way around the borrow checker here, but to no avail. This code also makes me sad, but at least it's all hidden behind the API. Someday we'll be able to do this better...

@Gankra
Copy link
Contributor Author

Gankra commented Oct 28, 2014

Sorry about that. Fixed, squashed, and make checked.

@Gankra
Copy link
Contributor Author

Gankra commented Oct 28, 2014

I don't understand the error that was spat out:

error: the trait `core::iter::FromIterator<(int,int)>` is not implemented for the type `smallintmap::SmallIntMap<int>`

let mut map: SmallIntMap<int> = xs.iter().map(|&x| x).collect();

I didn't rebase to latest master, could something odd have changed with resolution?

@aturon
Copy link
Member

aturon commented Oct 28, 2014

@gankro Method resolution has been in flux, so possibly? Definitely worth rebasing and seeing if you can reproduce.

@Gankra
Copy link
Contributor Author

Gankra commented Oct 29, 2014

Rebased to master, still works. I'm at a loss.

@reem
Copy link
Contributor

reem commented Oct 29, 2014

Is SmallIntMap really even worth having? It's basically just Vec.

@Gankra
Copy link
Contributor Author

Gankra commented Oct 29, 2014

@reem I've been pushing for stuff to be pulled when we start stabilizing. SmallIntMap I could take or leave. It's pretty easy for a client to write safely, but also has low maintenance cost.

Until then, I'm forced to assume it will stay.

@reem
Copy link
Contributor

reem commented Oct 29, 2014

What's the point? It offers almost no added utility over Vec<Option<T>>

@Gankra
Copy link
Contributor Author

Gankra commented Oct 30, 2014

@reem It's easier to use than a Vec<Option<T>>, and is faster to use than a heavier data structure. iirc rustc uses it. It's been suggested that some of these "rustc wants it" structures could be left in as #[experimental] so that we're making no promises to maintain/keep it outside. I dunno. Something we need to work out.

@aturon
Copy link
Member

aturon commented Oct 31, 2014

Even if we decide not to stabilize it, we'd move it out to an external Cargo crate, so I think it's still worth making the API consistent with the other collections.

@aturon
Copy link
Member

aturon commented Oct 31, 2014

@gankro I will try to take a look at this failure ASAP.

@Gankra
Copy link
Contributor Author

Gankra commented Nov 2, 2014

Killing this while collections reform is on.

@Gankra Gankra closed this Nov 2, 2014
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.

7 participants