Merge pull request #293 from cuviper/deprecate-remove

Re-deprecate `remove`, `remove_entry`, and `take`

README.md

@@ -26,7 +26,9 @@ was indexmap, a hash table that has following properties:
 - Order is **independent of hash function** and hash values of keys.
 - Fast to iterate.
 - Indexed in compact space.
-- Preserves insertion order **as long** as you don't call `.remove()`.
+- Preserves insertion order **as long** as you don't call `.remove()`,
+  `.swap_remove()`, or other methods that explicitly change order.
+  The alternate `.shift_remove()` does preserve relative order.
 - Uses hashbrown for the inner table, just like Rust's libstd `HashMap` does.
 
 ## Performance

src/map.rs

@@ -520,11 +520,12 @@ where
     /// Remove the key-value pair equivalent to `key` and return
     /// its value.
     ///
-    /// **NOTE:** This is equivalent to `.swap_remove(key)`, if you need to
-    /// preserve the order of the keys in the map, use `.shift_remove(key)`
-    /// instead.
-    ///
-    /// Computes in **O(1)** time (average).
+    /// **NOTE:** This is equivalent to [`.swap_remove(key)`][Self::swap_remove], replacing this
+    /// entry's position with the last element, and it is deprecated in favor of calling that
+    /// explicitly. If you need to preserve the relative order of the keys in the map, use
+    /// [`.shift_remove(key)`][Self::shift_remove] instead.
+    #[deprecated(note = "`remove` disrupts the map order -- \
+        use `swap_remove` or `shift_remove` for explicit behavior.")]
     pub fn remove<Q: ?Sized>(&mut self, key: &Q) -> Option<V>
     where
         Q: Hash + Equivalent<K>,
@@ -534,11 +535,12 @@ where
 
     /// Remove and return the key-value pair equivalent to `key`.
     ///
-    /// **NOTE:** This is equivalent to `.swap_remove_entry(key)`, if you need to
-    /// preserve the order of the keys in the map, use `.shift_remove_entry(key)`
-    /// instead.
-    ///
-    /// Computes in **O(1)** time (average).
+    /// **NOTE:** This is equivalent to [`.swap_remove_entry(key)`][Self::swap_remove_entry],
+    /// replacing this entry's position with the last element, and it is deprecated in favor of
+    /// calling that explicitly. If you need to preserve the relative order of the keys in the map,
+    /// use [`.shift_remove_entry(key)`][Self::shift_remove_entry] instead.
+    #[deprecated(note = "`remove_entry` disrupts the map order -- \
+        use `swap_remove_entry` or `shift_remove_entry` for explicit behavior.")]
     pub fn remove_entry<Q: ?Sized>(&mut self, key: &Q) -> Option<(K, V)>
     where
         Q: Hash + Equivalent<K>,

src/map/core.rs

@@ -664,7 +664,12 @@ impl<K, V> OccupiedEntry<'_, K, V> {
 
     /// Remove the key, value pair stored in the map for this entry, and return the value.
     ///
-    /// **NOTE:** This is equivalent to `.swap_remove()`.
+    /// **NOTE:** This is equivalent to [`.swap_remove()`][Self::swap_remove], replacing this
+    /// entry's position with the last element, and it is deprecated in favor of calling that
+    /// explicitly. If you need to preserve the relative order of the keys in the map, use
+    /// [`.shift_remove()`][Self::shift_remove] instead.
+    #[deprecated(note = "`remove` disrupts the map order -- \
+        use `swap_remove` or `shift_remove` for explicit behavior.")]
     pub fn remove(self) -> V {
         self.swap_remove()
     }
@@ -693,10 +698,39 @@ impl<K, V> OccupiedEntry<'_, K, V> {
 
     /// Remove and return the key, value pair stored in the map for this entry
     ///
-    /// **NOTE:** This is equivalent to `.swap_remove_entry()`.
+    /// **NOTE:** This is equivalent to [`.swap_remove_entry()`][Self::swap_remove_entry],
+    /// replacing this entry's position with the last element, and it is deprecated in favor of
+    /// calling that explicitly. If you need to preserve the relative order of the keys in the map,
+    /// use [`.shift_remove_entry()`][Self::shift_remove_entry] instead.
+    #[deprecated(note = "`remove_entry` disrupts the map order -- \
+        use `swap_remove_entry` or `shift_remove_entry` for explicit behavior.")]
     pub fn remove_entry(self) -> (K, V) {
         self.swap_remove_entry()
     }
+
+    /// Remove and return the key, value pair stored in the map for this entry
+    ///
+    /// Like `Vec::swap_remove`, the pair is removed by swapping it with the
+    /// last element of the map and popping it off. **This perturbs
+    /// the position of what used to be the last element!**
+    ///
+    /// Computes in **O(1)** time (average).
+    pub fn swap_remove_entry(self) -> (K, V) {
+        let (map, index) = self.remove_index();
+        map.swap_remove_finish(index)
+    }
+
+    /// Remove and return the key, value pair stored in the map for this entry
+    ///
+    /// Like `Vec::remove`, the pair is removed by shifting all of the
+    /// elements that follow it, preserving their relative order.
+    /// **This perturbs the index of all of those elements!**
+    ///
+    /// Computes in **O(n)** time (average).
+    pub fn shift_remove_entry(self) -> (K, V) {
+        let (map, index) = self.remove_index();
+        map.shift_remove_finish(index)
+    }
 }
 
 impl<K: fmt::Debug, V: fmt::Debug> fmt::Debug for OccupiedEntry<'_, K, V> {

src/map/core/raw.rs

@@ -143,52 +143,32 @@ impl<'a, K, V> OccupiedEntry<'a, K, V> {
         &mut self.map.entries[index].value
     }
 
-    /// Put the new key in the occupied entry's key slot
-    pub(crate) fn replace_key(self) -> K {
+    /// Converts into a mutable reference to the entry's value in the map,
+    /// with a lifetime bound to the map itself.
+    pub fn into_mut(self) -> &'a mut V {
         let index = self.index();
-        let old_key = &mut self.map.entries[index].key;
-        replace(old_key, self.key)
+        &mut self.map.entries[index].value
     }
 
     /// Return the index of the key-value pair
     #[inline]
     pub fn index(&self) -> usize {
-        // SAFETY: we have &mut map keep keeping the bucket stable
+        // SAFETY: we have `&mut map` keeping the bucket stable
         unsafe { *self.raw_bucket.as_ref() }
     }
 
-    /// Converts into a mutable reference to the entry's value in the map,
-    /// with a lifetime bound to the map itself.
-    pub fn into_mut(self) -> &'a mut V {
+    /// Put the new key in the occupied entry's key slot
+    pub(crate) fn replace_key(self) -> K {
         let index = self.index();
-        &mut self.map.entries[index].value
-    }
-
-    /// Remove and return the key, value pair stored in the map for this entry
-    ///
-    /// Like `Vec::swap_remove`, the pair is removed by swapping it with the
-    /// last element of the map and popping it off. **This perturbs
-    /// the position of what used to be the last element!**
-    ///
-    /// Computes in **O(1)** time (average).
-    pub fn swap_remove_entry(self) -> (K, V) {
-        // SAFETY: This is safe because it can only happen once (self is consumed)
-        // and map.indices have not been modified since entry construction
-        let (index, _slot) = unsafe { self.map.indices.remove(self.raw_bucket) };
-        self.map.swap_remove_finish(index)
+        let old_key = &mut self.map.entries[index].key;
+        replace(old_key, self.key)
     }
 
-    /// Remove and return the key, value pair stored in the map for this entry
-    ///
-    /// Like `Vec::remove`, the pair is removed by shifting all of the
-    /// elements that follow it, preserving their relative order.
-    /// **This perturbs the index of all of those elements!**
-    ///
-    /// Computes in **O(n)** time (average).
-    pub fn shift_remove_entry(self) -> (K, V) {
+    /// Remove the index from indices, leaving the actual entries to the caller.
+    pub(super) fn remove_index(self) -> (&'a mut IndexMapCore<K, V>, usize) {
         // SAFETY: This is safe because it can only happen once (self is consumed)
         // and map.indices have not been modified since entry construction
         let (index, _slot) = unsafe { self.map.indices.remove(self.raw_bucket) };
-        self.map.shift_remove_finish(index)
+        (self.map, index)
     }
 }

src/set.rs

@@ -463,10 +463,12 @@ where
 
     /// Remove the value from the set, and return `true` if it was present.
     ///
-    /// **NOTE:** This is equivalent to `.swap_remove(value)`, if you want
-    /// to preserve the order of the values in the set, use `.shift_remove(value)`.
-    ///
-    /// Computes in **O(1)** time (average).
+    /// **NOTE:** This is equivalent to [`.swap_remove(value)`][Self::swap_remove], replacing this
+    /// value's position with the last element, and it is deprecated in favor of calling that
+    /// explicitly. If you need to preserve the relative order of the values in the set, use
+    /// [`.shift_remove(value)`][Self::shift_remove] instead.
+    #[deprecated(note = "`remove` disrupts the set order -- \
+        use `swap_remove` or `shift_remove` for explicit behavior.")]
     pub fn remove<Q: ?Sized>(&mut self, value: &Q) -> bool
     where
         Q: Hash + Equivalent<T>,
@@ -509,11 +511,12 @@ where
     /// Removes and returns the value in the set, if any, that is equal to the
     /// given one.
     ///
-    /// **NOTE:** This is equivalent to `.swap_take(value)`, if you need to
-    /// preserve the order of the values in the set, use `.shift_take(value)`
-    /// instead.
-    ///
-    /// Computes in **O(1)** time (average).
+    /// **NOTE:** This is equivalent to [`.swap_take(value)`][Self::swap_take], replacing this
+    /// value's position with the last element, and it is deprecated in favor of calling that
+    /// explicitly. If you need to preserve the relative order of the values in the set, use
+    /// [`.shift_take(value)`][Self::shift_take] instead.
+    #[deprecated(note = "`take` disrupts the set order -- \
+        use `swap_take` or `shift_take` for explicit behavior.")]
     pub fn take<Q: ?Sized>(&mut self, value: &Q) -> Option<T>
     where
         Q: Hash + Equivalent<T>,