auto merge of #5796 : nikomatsakis/rust/issue-5656-fix-map-iteration, r=nikomatsakis

Revert map.each to something which takes two parameters rather than a tuple.  The current setup iterates over `BaseIter<(&'self K, &'self V)>` where 'self is a lifetime declared *in the `each()` method*.  You can't place such a type in the impl declaration.  The compiler currently allows it, but this will not be legal under #5656 and I'm pretty sure it's not sound now.  It's too bad that maps can't implement `BaseIter` (at least not over a tuple as they do here) but I think it has to be this way for the time being.

r? @thestinger

Author: bors

src/libcore/container.rs

@@ -29,6 +29,9 @@ pub trait Map<K, V>: Mutable {
     /// Return true if the map contains a value for the specified key
     fn contains_key(&self, key: &K) -> bool;
 
+    // Visits all keys and values
+    fn each(&self, f: &fn(&K, &V) -> bool);
+
     /// Visit all keys
     fn each_key(&self, f: &fn(&K) -> bool);
 

src/libcore/hashmap.rs

@@ -279,24 +279,6 @@ priv impl<K:Hash + IterBytes + Eq,V> HashMap<K, V> {
     }
 }
 
-impl<'self,K:Hash + IterBytes + Eq,V>
-        BaseIter<(&'self K, &'self V)> for HashMap<K, V> {
-    /// Visit all key-value pairs
-    fn each(&self, blk: &fn(&(&'self K, &'self V)) -> bool) {
-        for uint::range(0, self.buckets.len()) |i| {
-            let mut broke = false;
-            do self.buckets[i].map |bucket| {
-                if !blk(&(&bucket.key, &bucket.value)) {
-                    broke = true; // FIXME(#3064) just write "break;"
-                }
-            };
-            if broke { break; }
-        }
-    }
-    fn size_hint(&self) -> Option<uint> { Some(self.len()) }
-}
-
-
 impl<K:Hash + IterBytes + Eq,V> Container for HashMap<K, V> {
     /// Return the number of elements in the map
     fn len(&const self) -> uint { self.size }
@@ -315,7 +297,7 @@ impl<K:Hash + IterBytes + Eq,V> Mutable for HashMap<K, V> {
     }
 }
 
-impl<'self,K:Hash + IterBytes + Eq,V> Map<K, V> for HashMap<K, V> {
+impl<K:Hash + IterBytes + Eq,V> Map<K, V> for HashMap<K, V> {
     /// Return true if the map contains a value for the specified key
     fn contains_key(&self, k: &K) -> bool {
         match self.bucket_for_key(k) {
@@ -324,14 +306,25 @@ impl<'self,K:Hash + IterBytes + Eq,V> Map<K, V> for HashMap<K, V> {
         }
     }
 
+    /// Visit all key-value pairs
+    fn each(&self, blk: &fn(&'self K, &'self V) -> bool) {
+        for uint::range(0, self.buckets.len()) |i| {
+            for self.buckets[i].each |bucket| {
+                if !blk(&bucket.key, &bucket.value) {
+                    return;
+                }
+            }
+        }
+    }
+
     /// Visit all keys
     fn each_key(&self, blk: &fn(k: &K) -> bool) {
-        self.each(|&(k, _)| blk(k))
+        self.each(|k, _| blk(k))
     }
 
     /// Visit all values
     fn each_value(&self, blk: &fn(v: &V) -> bool) {
-        self.each(|&(_, v)| blk(v))
+        self.each(|_, v| blk(v))
     }
 
     /// Iterate over the map and mutate the contained values
@@ -545,7 +538,7 @@ impl<K:Hash + IterBytes + Eq,V:Eq> Eq for HashMap<K, V> {
     fn eq(&self, other: &HashMap<K, V>) -> bool {
         if self.len() != other.len() { return false; }
 
-        for self.each |&(key, value)| {
+        for self.each |key, value| {
             match other.find(key) {
                 None => return false,
                 Some(v) => if value != v { return false },
@@ -798,7 +791,7 @@ mod test_map {
             assert!(m.insert(i, i*2));
         }
         let mut observed = 0;
-        for m.each |&(k, v)| {
+        for m.each |k, v| {
             assert!(*v == *k * 2);
             observed |= (1 << *k);
         }

src/libcore/trie.rs

@@ -28,24 +28,6 @@ pub struct TrieMap<T> {
     priv length: uint
 }
 
-impl<'self,T> BaseIter<(uint, &'self T)> for TrieMap<T> {
-    /// Visit all key-value pairs in order
-    #[inline(always)]
-    fn each(&self, f: &fn(&(uint, &'self T)) -> bool) {
-        self.root.each(f);
-    }
-    #[inline(always)]
-    fn size_hint(&self) -> Option<uint> { Some(self.len()) }
-}
-
-impl<'self,T> ReverseIter<(uint, &'self T)> for TrieMap<T> {
-    /// Visit all key-value pairs in reverse order
-    #[inline(always)]
-    fn each_reverse(&self, f: &fn(&(uint, &'self T)) -> bool) {
-        self.root.each_reverse(f);
-    }
-}
-
 impl<T> Container for TrieMap<T> {
     /// Return the number of elements in the map
     #[inline(always)]
@@ -72,16 +54,22 @@ impl<T> Map<uint, T> for TrieMap<T> {
         self.find(key).is_some()
     }
 
+    /// Visit all key-value pairs in order
+    #[inline(always)]
+    fn each(&self, f: &fn(&uint, &'self T) -> bool) {
+        self.root.each(f);
+    }
+
     /// Visit all keys in order
     #[inline(always)]
     fn each_key(&self, f: &fn(&uint) -> bool) {
-        self.each(|&(k, _)| f(&k))
+        self.each(|k, _| f(k))
     }
 
     /// Visit all values in order
     #[inline(always)]
     fn each_value(&self, f: &fn(&T) -> bool) {
-        self.each(|&(_, v)| f(v))
+        self.each(|_, v| f(v))
     }
 
     /// Iterate over the map and mutate the contained values
@@ -148,16 +136,22 @@ pub impl<T> TrieMap<T> {
         TrieMap{root: TrieNode::new(), length: 0}
     }
 
+    /// Visit all key-value pairs in reverse order
+    #[inline(always)]
+    fn each_reverse(&self, f: &fn(&uint, &'self T) -> bool) {
+        self.root.each_reverse(f);
+    }
+
     /// Visit all keys in reverse order
     #[inline(always)]
     fn each_key_reverse(&self, f: &fn(&uint) -> bool) {
-        self.each_reverse(|&(k, _)| f(&k))
+        self.each_reverse(|k, _| f(k))
     }
 
     /// Visit all values in reverse order
     #[inline(always)]
     fn each_value_reverse(&self, f: &fn(&T) -> bool) {
-        self.each_reverse(|&(_, v)| f(v))
+        self.each_reverse(|_, v| f(v))
     }
 }
 
@@ -239,22 +233,22 @@ impl<T> TrieNode<T> {
 }
 
 impl<T> TrieNode<T> {
-    fn each(&self, f: &fn(&(uint, &'self T)) -> bool) -> bool {
+    fn each(&self, f: &fn(&uint, &'self T) -> bool) -> bool {
         for uint::range(0, self.children.len()) |idx| {
             match self.children[idx] {
                 Internal(ref x) => if !x.each(f) { return false },
-                External(k, ref v) => if !f(&(k, v)) { return false },
+                External(k, ref v) => if !f(&k, v) { return false },
                 Nothing => ()
             }
         }
         true
     }
 
-    fn each_reverse(&self, f: &fn(&(uint, &'self T)) -> bool) -> bool {
+    fn each_reverse(&self, f: &fn(&uint, &'self T) -> bool) -> bool {
         for uint::range_rev(self.children.len(), 0) |idx| {
             match self.children[idx - 1] {
                 Internal(ref x) => if !x.each_reverse(f) { return false },
-                External(k, ref v) => if !f(&(k, v)) { return false },
+                External(k, ref v) => if !f(&k, v) { return false },
                 Nothing => ()
             }
         }
@@ -438,8 +432,8 @@ mod tests {
         assert!(m.insert(1, 2));
 
         let mut n = 0;
-        for m.each |&(k, v)| {
-            assert!(k == n);
+        for m.each |k, v| {
+            assert!(*k == n);
             assert!(*v == n * 2);
             n += 1;
         }
@@ -454,11 +448,11 @@ mod tests {
         }
 
         let mut n = uint::max_value - 9999;
-        for m.each |&(k, v)| {
+        for m.each |k, v| {
             if n == uint::max_value - 5000 { break }
             assert!(n < uint::max_value - 5000);
 
-            assert!(k == n);
+            assert!(*k == n);
             assert!(*v == n / 2);
             n += 1;
         }
@@ -475,8 +469,8 @@ mod tests {
         assert!(m.insert(1, 2));
 
         let mut n = 4;
-        for m.each_reverse |&(k, v)| {
-            assert!(k == n);
+        for m.each_reverse |k, v| {
+            assert!(*k == n);
             assert!(*v == n * 2);
             n -= 1;
         }
@@ -491,11 +485,11 @@ mod tests {
         }
 
         let mut n = uint::max_value;
-        for m.each_reverse |&(k, v)| {
+        for m.each_reverse |k, v| {
             if n == uint::max_value - 5000 { break }
             assert!(n > uint::max_value - 5000);
 
-            assert!(k == n);
+            assert!(*k == n);
             assert!(*v == n / 2);
             n -= 1;
         }

src/librustc/metadata/cstore.rs

@@ -86,7 +86,7 @@ pub fn have_crate_data(cstore: &CStore, cnum: ast::crate_num) -> bool {
 
 pub fn iter_crate_data(cstore: &CStore,
                        i: &fn(ast::crate_num, @crate_metadata)) {
-    for cstore.metas.each |&(&k, &v)| {
+    for cstore.metas.each |&k, &v| {
         i(k, v);
     }
 }

src/librustc/middle/lang_items.rs

@@ -397,7 +397,7 @@ pub impl<'self> LanguageItemCollector<'self> {
     }
 
     fn check_completeness(&self) {
-        for self.item_refs.each |&(&key, &item_ref)| {
+        for self.item_refs.each |&key, &item_ref| {
             match self.items.items[item_ref] {
                 None => {
                     self.session.err(fmt!("no item found for `%s`", *key));

src/librustc/middle/lint.rs

@@ -460,7 +460,7 @@ pub fn build_settings_crate(sess: session::Session, crate: @ast::crate) {
 
     do cx.with_lint_attrs(/*bad*/copy crate.node.attrs) |cx| {
         // Copy out the default settings
-        for cx.curr.each |&(k, &v)| {
+        for cx.curr.each |&k, &v| {
             sess.lint_settings.default_settings.insert(k, v);
         }
 

src/librustc/middle/region.rs

@@ -842,7 +842,7 @@ pub fn determine_rp_in_crate(sess: Session,
     debug!("%s", {
         debug!("Region variance results:");
         let region_paramd_items = cx.region_paramd_items;
-        for region_paramd_items.each |&(&key, &value)| {
+        for region_paramd_items.each |&key, &value| {
             debug!("item %? (%s) is parameterized with variance %?",
                    key,
                    ast_map::node_id_to_str(ast_map, key,

src/librustc/middle/resolve.rs

@@ -2369,7 +2369,7 @@ pub impl Resolver {
 
         // Add all resolved imports from the containing module.
         for containing_module.import_resolutions.each
-                |&(ident, target_import_resolution)| {
+                |ident, target_import_resolution| {
 
             debug!("(resolving glob import) writing module resolution \
                     %? into `%s`",
@@ -2457,13 +2457,13 @@ pub impl Resolver {
         };
 
         // Add all children from the containing module.
-        for containing_module.children.each |&(ident, name_bindings)| {
+        for containing_module.children.each |ident, name_bindings| {
             merge_import_resolution(ident, *name_bindings);
         }
 
         // Add external module children from the containing module.
         for containing_module.external_module_children.each
-                |&(ident, module)| {
+                |ident, module| {
             let name_bindings =
                 @mut Resolver::create_name_bindings_from_module(*module);
             merge_import_resolution(ident, name_bindings);
@@ -3111,7 +3111,7 @@ pub impl Resolver {
     fn add_exports_for_module(@mut self,
                               exports2: &mut ~[Export2],
                               module_: @mut Module) {
-        for module_.children.each |&(ident, namebindings)| {
+        for module_.children.each |ident, namebindings| {
             debug!("(computing exports) maybe export '%s'",
                    *self.session.str_of(*ident));
             self.add_exports_of_namebindings(&mut *exports2,
@@ -3126,7 +3126,7 @@ pub impl Resolver {
                                              false);
         }
 
-        for module_.import_resolutions.each |&(ident, importresolution)| {
+        for module_.import_resolutions.each |ident, importresolution| {
             if importresolution.privacy != Public {
                 debug!("(computing exports) not reexporting private `%s`",
                        *self.session.str_of(*ident));
@@ -3934,7 +3934,7 @@ pub impl Resolver {
         for arm.pats.eachi() |i, p| {
             let map_i = self.binding_mode_map(*p);
 
-            for map_0.each |&(&key, &binding_0)| {
+            for map_0.each |&key, &binding_0| {
                 match map_i.find(&key) {
                   None => {
                     self.session.span_err(
@@ -3955,7 +3955,7 @@ pub impl Resolver {
                 }
             }
 
-            for map_i.each |&(&key, &binding)| {
+            for map_i.each |&key, &binding| {
                 if !map_0.contains_key(&key) {
                     self.session.span_err(
                         binding.span,
@@ -5248,7 +5248,7 @@ pub impl Resolver {
         }
 
         debug!("Import resolutions:");
-        for module_.import_resolutions.each |&(name, import_resolution)| {
+        for module_.import_resolutions.each |name, import_resolution| {
             let mut value_repr;
             match import_resolution.target_for_namespace(ValueNS) {
                 None => { value_repr = ~""; }

src/librustc/middle/trans/base.rs

@@ -2848,7 +2848,7 @@ pub fn create_module_map(ccx: @CrateContext) -> ValueRef {
         lib::llvm::SetLinkage(map, lib::llvm::InternalLinkage);
     }
     let mut elts: ~[ValueRef] = ~[];
-    for ccx.module_data.each |&(key, &val)| {
+    for ccx.module_data.each |key, &val| {
         let elt = C_struct(~[p2i(ccx, C_cstr(ccx, @/*bad*/ copy *key)),
                             p2i(ccx, val)]);
         elts.push(elt);
@@ -3139,7 +3139,7 @@ pub fn trans_crate(sess: session::Session,
         }
 
         if ccx.sess.count_llvm_insns() {
-            for ccx.stats.llvm_insns.each |&(&k, &v)| {
+            for ccx.stats.llvm_insns.each |&k, &v| {
                 io::println(fmt!("%-7u %s", v, k));
             }
         }

src/librustc/middle/typeck/infer/region_inference.rs

@@ -1223,7 +1223,7 @@ pub impl RegionVarBindings {
 
         // It would be nice to write this using map():
         let mut edges = vec::with_capacity(num_edges);
-        for self.constraints.each |&(constraint, span)| {
+        for self.constraints.each |constraint, span| {
             edges.push(GraphEdge {
                 next_edge: [uint::max_value, uint::max_value],
                 constraint: *constraint,

src/librustc/rustc.rc

@@ -177,7 +177,7 @@ Available lint options:
                      padded(max_key, ~"name"), ~"default", ~"meaning"));
     io::println(fmt!("    %s  %7.7s  %s\n",
                      padded(max_key, ~"----"), ~"-------", ~"-------"));
-    for lint_dict.each |&(k, v)| {
+    for lint_dict.each |k, v| {
         let k = str::replace(*k, ~"_", ~"-");
         io::println(fmt!("    %s  %7.7s  %s",
                          padded(max_key, k),