Skip to content

auto: oldmap: use &K instead of K in find and get #4777

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

Merged
merged 1 commit into from
Feb 5, 2013
Merged

auto: oldmap: use &K instead of K in find and get #4777

merged 1 commit into from
Feb 5, 2013

Conversation

thestinger
Copy link
Contributor

continuing the work from #4746

@alexcrichton
Copy link
Member

Just curious, but is there a vision at some point for making this nice for keys which can actually be copied? Before I was able to have things like:

let map : HashMap<int, int> = ...;
let bar = 2;
// Don't need the address to bar, and also implements
// the Index<K, V> trait which would also be nice
let foo = map[bar];
// Perhaps there could be an trait for 'foo[k] = v' syntax?
map.insert(foo, 3);
// Don't need the address here again because '&4' looks kinda silly
map.remove(4);

I do agree that for most of rust it's necessary to have &K instead of K, so I was just wondering if this was ever going to manifest itself in some form or fashion. I think it pretty much always make sense to return &V as opposed to V, but sometimes it's far nicer (and just makes more sense) to use K as the key on all the functions instead of &K. If this could be a choice of the user of the map as opposed to one decided ahead of time, that would be awesome (or maybe some other map/trait pair?)

I took a look at the Map trait to see if things like LinearMap could implement Map<&K, &V> whereas something like SmallIntMap would implement Map<int, &V> or something like that. It didn't look like it'd be "easy" to do because insert takes K while remove takes &K and &&K makes no sense.

Again, just a general question, I think the container work you've been doing is awesome and things are becoming far more unified than when I started to use rust (0.5 release as comparison)

@thestinger
Copy link
Contributor Author

@alexcrichton: It's a little bit uglier for small implicitly copyable types (the built-in integer types), but there isn't a performance loss and I can't think of any better alternatives. The insert method still takes the parameters by-value since it needs ownership, but the find and remove methods use &K because that's all they need.

Using & on generic type parameters when ownership isn't required provides good support for types that can't be copied or cloned, and it avoids potentially expensive copies/clones for ones that can be. For the built-in numeric types it's just a bit of syntactic noise instead of better semantics though. The other traits in the standard library are similar, and C++/D do it this way too (although they use implicit pass-by-reference so it's less noticeable).

@alexcrichton
Copy link
Member

Oh I completely understand why it's the wait it is, I was just curious if there were any visions for making this syntactically nicer or somehow controllable. Thanks for the explanation!

@bors bors closed this Feb 5, 2013
@bors bors merged commit 8e64352 into rust-lang:incoming Feb 5, 2013
@thestinger thestinger deleted the find branch February 5, 2013 03:31
@nikomatsakis
Copy link
Contributor

@alexcrichton One option might be to make an alternate map interface that is intended for copyable keys which is implemented for all maps, like:

trait MapCopy<K:Copy, V> {
    fn get_copy(&'lt self, k: K) -> &'lt V;
    ...
}

impl<K:Copy,V,M:Map<K,V>> MapCopy<K,V> for M {
    fn get_copy(&'lt self, K: K) -> &'lt V {
        self.get(&k)
    }
}

Probably we would want to find a nicer name than get_copy(). Maybe just get(), and then in your code you would want to have either Map or MapCopy in scope, but not both. Anyway, something like that could work.

@alexcrichton
Copy link
Member

Hmm... I wouldn't be in favor of something like get_copy because the _copy is already more characters than a & symbol anyway. Disallowing them to be used at the same time might also make things difficult.

Would something like this be out of the question?

pub trait Container {
    // I was always more in favor of 'size' because I only really consider
    // vectors as having a 'length'
    pure fn len(&self) -> uint;
    pure fn is_empty(&self) -> bool;
}

pub trait Mutable: Container {
    fn clear(&mut self);
}

pub trait Map<K, V>: Mutable {
    pure fn contains_key(&self, key: K) -> bool;
    pure fn each(&self, f: fn(K, V) -> bool);
    pure fn each_key(&self, f: fn(K) -> bool);
    pure fn each_value(&self, f: fn(V) -> bool);
    pure fn find(&self, key: K) -> Option<V>;
    fn remove(&mut self, key: K) -> bool;
}

pub trait GrowableMap<K, V> {
    fn insert(&mut self, key: K, value: V) -> bool;
}

impl<K, V> Map<&K, &V> for LinearMap<K, V> { ... }
impl<K, V> GrowableMap<K, V> for LinearMap<K, V> { ... }

impl<V> Map<int, &V> for SmallIntMap<V> { ... }
impl<V> GrowableMap<int, V> for SmallIntMap<V> { ... }

It's kind of an odd workaround, but the insert method is the only one which needs to be different from all the other ones. With this kind of trait all SmallIntMap instances wouldn't need &uint. One of the other problems is that if you wanted to add like the pop method onto the Map trait, you'd have to have another PoppableMap trait which doesn't quite make sense and just looks silly to have N traits for N methods:

pub trait PoppableMap<K, V> {
    fn pop(&mut self, key: K) -> Option<V>;
}

impl<K, V> PoppableMap<&K, V> for LinearMap<K, V> { ... }
impl<V> PoppableMap<uint, V> for SmallIntMap<V> { ... }

I'm not sure if this is even possible. I don't quite have a complete grasp on where methods go and how you access them when dealing with traits, so this could just not even work. Regardless, I was just looking into if there was an easy way to use the maps as naturally as possible. When I first started using rust I was really confused that I had to write &~"str" to key into a one of json's Object maps, but at some point you just get used to it and it's just a rust thing (not that any of this would fix that anyway).

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.

4 participants