From 0247a1555de940982260384101946d3f958452e5 Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Tue, 27 Aug 2024 15:48:17 -0700 Subject: [PATCH 1/4] Document and assert index bounds in `shift_insert` --- src/map.rs | 8 +++++++- src/set.rs | 11 +++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/map.rs b/src/map.rs index b224aace..283970d2 100644 --- a/src/map.rs +++ b/src/map.rs @@ -448,26 +448,32 @@ where /// Insert a key-value pair in the map at the given index. /// /// If an equivalent key already exists in the map: the key remains and - /// is moved to the new position in the map, its corresponding value is updated + /// is moved to the given index in the map, its corresponding value is updated /// with `value`, and the older value is returned inside `Some(_)`. + /// Note that existing entries **cannot** be moved to `index == map.len()`! /// /// If no equivalent key existed in the map: the new key-value pair is /// inserted at the given index, and `None` is returned. /// /// ***Panics*** if `index` is out of bounds. + /// Valid indices are `0..map.len()` (exclusive) when moving an existing entry, or + /// `0..=map.len()` (inclusive) when inserting a new key. /// /// Computes in **O(n)** time (average). /// /// See also [`entry`][Self::entry] if you want to insert *or* modify, /// perhaps only using the index for new entries with [`VacantEntry::shift_insert`]. pub fn shift_insert(&mut self, index: usize, key: K, value: V) -> Option { + let len = self.len(); match self.entry(key) { Entry::Occupied(mut entry) => { + assert!(index < len, "index out of bounds"); let old = mem::replace(entry.get_mut(), value); entry.move_index(index); Some(old) } Entry::Vacant(entry) => { + assert!(index <= len, "index out of bounds"); entry.shift_insert(index, value); None } diff --git a/src/set.rs b/src/set.rs index 7a8ac4df..1f5f84f9 100644 --- a/src/set.rs +++ b/src/set.rs @@ -385,12 +385,15 @@ where /// Insert the value into the set at the given index. /// - /// If an equivalent item already exists in the set, it returns - /// `false` leaving the original value in the set, but moving it to - /// the new position in the set. Otherwise, it inserts the new - /// item at the given index and returns `true`. + /// If an equivalent item already exists in the set, it returns `false` leaving + /// the original value in the set, but moved to the given index. + /// Note that existing values **cannot** be moved to `index == set.len()`! + /// + /// Otherwise, it inserts the new value at the given index and returns `true`. /// /// ***Panics*** if `index` is out of bounds. + /// Valid indices are `0..set.len()` (exclusive) when moving an existing value, or + /// `0..=set.len()` (inclusive) when inserting a new value. /// /// Computes in **O(n)** time (average). pub fn shift_insert(&mut self, index: usize, value: T) -> bool { From 7224def0106a4a6074a0d4619ce99d4120b859a8 Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Tue, 27 Aug 2024 15:51:27 -0700 Subject: [PATCH 2/4] Add `insert_before` as an alternate to `shift_insert` --- src/map.rs | 40 ++++++++++++++++++++++++++++++++++++++++ src/set.rs | 19 +++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/src/map.rs b/src/map.rs index 283970d2..d8741f1d 100644 --- a/src/map.rs +++ b/src/map.rs @@ -445,12 +445,52 @@ where } } + /// Insert a key-value pair in the map before the entry at the given index, or at the end. + /// + /// If an equivalent key already exists in the map: the key remains and + /// is moved to the new position in the map, its corresponding value is updated + /// with `value`, and the older value is returned inside `Some(_)`. The returned index + /// will either be the given index or one less, depending on how the entry moved. + /// (See [`shift_insert`](Self::shift_insert) for different behavior here.) + /// + /// If no equivalent key existed in the map: the new key-value pair is + /// inserted exactly at the given index, and `None` is returned. + /// + /// ***Panics*** if `index` is out of bounds. + /// Valid indices are `0..=map.len()` (inclusive). + /// + /// Computes in **O(n)** time (average). + /// + /// See also [`entry`][Self::entry] if you want to insert *or* modify, + /// perhaps only using the index for new entries with [`VacantEntry::shift_insert`]. + pub fn insert_before(&mut self, mut index: usize, key: K, value: V) -> (usize, Option) { + assert!(index <= self.len(), "index out of bounds"); + match self.entry(key) { + Entry::Occupied(mut entry) => { + if index > entry.index() { + // Some entries will shift down when this one moves up, + // so "insert before index" becomes "move to index - 1", + // keeping the entry at the original index unmoved. + index -= 1; + } + let old = mem::replace(entry.get_mut(), value); + entry.move_index(index); + (index, Some(old)) + } + Entry::Vacant(entry) => { + entry.shift_insert(index, value); + (index, None) + } + } + } + /// Insert a key-value pair in the map at the given index. /// /// If an equivalent key already exists in the map: the key remains and /// is moved to the given index in the map, its corresponding value is updated /// with `value`, and the older value is returned inside `Some(_)`. /// Note that existing entries **cannot** be moved to `index == map.len()`! + /// (See [`insert_before`](Self::insert_before) for different behavior here.) /// /// If no equivalent key existed in the map: the new key-value pair is /// inserted at the given index, and `None` is returned. diff --git a/src/set.rs b/src/set.rs index 1f5f84f9..b1603047 100644 --- a/src/set.rs +++ b/src/set.rs @@ -383,11 +383,30 @@ where (index, existing.is_none()) } + /// Insert the value into the set before the value at the given index, or at the end. + /// + /// If an equivalent item already exists in the set, it returns `false` leaving the + /// original value in the set, but moved to the new position. The returned index + /// will either be the given index or one less, depending on how the value moved. + /// (See [`shift_insert`](Self::shift_insert) for different behavior here.) + /// + /// Otherwise, it inserts the new value exactly at the given index and returns `true`. + /// + /// ***Panics*** if `index` is out of bounds. + /// Valid indices are `0..=set.len()` (inclusive). + /// + /// Computes in **O(n)** time (average). + pub fn insert_before(&mut self, index: usize, value: T) -> (usize, bool) { + let (index, existing) = self.map.insert_before(index, value, ()); + (index, existing.is_none()) + } + /// Insert the value into the set at the given index. /// /// If an equivalent item already exists in the set, it returns `false` leaving /// the original value in the set, but moved to the given index. /// Note that existing values **cannot** be moved to `index == set.len()`! + /// (See [`insert_before`](Self::insert_before) for different behavior here.) /// /// Otherwise, it inserts the new value at the given index and returns `true`. /// From 8ca01b0df72a4914b2248a65087ce67e3711f52d Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Tue, 27 Aug 2024 15:52:26 -0700 Subject: [PATCH 3/4] Use `insert_before` for "new" entries in `insert_sorted` The only difference compared to using `shift_insert` is when the binary-searched key isn't *actually* new to the map, just not found in properly sorted order. In this case we can't guarantee a sorted result either, but it will at least behave better about the new position, especially if that's the end. --- src/map.rs | 4 ++-- src/map/tests.rs | 28 ++++++++++++++++++++++++++++ src/set.rs | 2 +- 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/map.rs b/src/map.rs index d8741f1d..7a7b6c2b 100644 --- a/src/map.rs +++ b/src/map.rs @@ -420,7 +420,7 @@ where /// /// This is equivalent to finding the position with /// [`binary_search_keys`][Self::binary_search_keys], then either updating - /// it or calling [`shift_insert`][Self::shift_insert] for a new key. + /// it or calling [`insert_before`][Self::insert_before] for a new key. /// /// If the sorted key is found in the map, its corresponding value is /// updated with `value`, and the older value is returned inside @@ -441,7 +441,7 @@ where { match self.binary_search_keys(&key) { Ok(i) => (i, Some(mem::replace(&mut self[i], value))), - Err(i) => (i, self.shift_insert(i, key, value)), + Err(i) => self.insert_before(i, key, value), } } diff --git a/src/map/tests.rs b/src/map/tests.rs index 0c7d93b3..cbd4b4f2 100644 --- a/src/map/tests.rs +++ b/src/map/tests.rs @@ -135,6 +135,34 @@ fn shift_insert() { } } +#[test] +fn insert_sorted_bad() { + let mut map = IndexMap::new(); + map.insert(10, ()); + for i in 0..10 { + map.insert(i, ()); + } + + // The binary search will want to insert this at the end (index == len()), + // but that's only possible for *new* inserts. It should still be handled + // without panicking though, and in this case it's simple enough that we + // know the exact result. (But don't read this as an API guarantee!) + assert_eq!(map.first(), Some((&10, &()))); + map.insert_sorted(10, ()); + assert_eq!(map.last(), Some((&10, &()))); + assert!(map.keys().copied().eq(0..=10)); + + // Other out-of-order entries can also "insert" to a binary-searched + // position, moving in either direction. + map.move_index(5, 0); + map.move_index(6, 10); + assert_eq!(map.first(), Some((&5, &()))); + assert_eq!(map.last(), Some((&6, &()))); + map.insert_sorted(5, ()); // moves back up + map.insert_sorted(6, ()); // moves back down + assert!(map.keys().copied().eq(0..=10)); +} + #[test] fn grow() { let insert = [0, 4, 2, 12, 8, 7, 11]; diff --git a/src/set.rs b/src/set.rs index b1603047..c7b97502 100644 --- a/src/set.rs +++ b/src/set.rs @@ -361,7 +361,7 @@ where /// /// This is equivalent to finding the position with /// [`binary_search`][Self::binary_search], and if needed calling - /// [`shift_insert`][Self::shift_insert] for a new value. + /// [`insert_before`][Self::insert_before] for a new value. /// /// If the sorted item is found in the set, it returns the index of that /// existing item and `false`, without any change. Otherwise, it inserts the From 1d9b5e3d0345aacf296eec8515746b3dfb81f97d Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Thu, 29 Aug 2024 16:58:37 -0700 Subject: [PATCH 4/4] Add doc examples for `insert_before` and `shift_insert` --- src/map.rs | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/set.rs | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+) diff --git a/src/map.rs b/src/map.rs index 7a7b6c2b..019c7f6d 100644 --- a/src/map.rs +++ b/src/map.rs @@ -463,6 +463,36 @@ where /// /// See also [`entry`][Self::entry] if you want to insert *or* modify, /// perhaps only using the index for new entries with [`VacantEntry::shift_insert`]. + /// + /// # Examples + /// + /// ``` + /// use indexmap::IndexMap; + /// let mut map: IndexMap = ('a'..='z').map(|c| (c, ())).collect(); + /// + /// // The new key '*' goes exactly at the given index. + /// assert_eq!(map.get_index_of(&'*'), None); + /// assert_eq!(map.insert_before(10, '*', ()), (10, None)); + /// assert_eq!(map.get_index_of(&'*'), Some(10)); + /// + /// // Moving the key 'a' up will shift others down, so this moves *before* 10 to index 9. + /// assert_eq!(map.insert_before(10, 'a', ()), (9, Some(()))); + /// assert_eq!(map.get_index_of(&'a'), Some(9)); + /// assert_eq!(map.get_index_of(&'*'), Some(10)); + /// + /// // Moving the key 'z' down will shift others up, so this moves to exactly 10. + /// assert_eq!(map.insert_before(10, 'z', ()), (10, Some(()))); + /// assert_eq!(map.get_index_of(&'z'), Some(10)); + /// assert_eq!(map.get_index_of(&'*'), Some(11)); + /// + /// // Moving or inserting before the endpoint is also valid. + /// assert_eq!(map.len(), 27); + /// assert_eq!(map.insert_before(map.len(), '*', ()), (26, Some(()))); + /// assert_eq!(map.get_index_of(&'*'), Some(26)); + /// assert_eq!(map.insert_before(map.len(), '+', ()), (27, None)); + /// assert_eq!(map.get_index_of(&'+'), Some(27)); + /// assert_eq!(map.len(), 28); + /// ``` pub fn insert_before(&mut self, mut index: usize, key: K, value: V) -> (usize, Option) { assert!(index <= self.len(), "index out of bounds"); match self.entry(key) { @@ -503,6 +533,44 @@ where /// /// See also [`entry`][Self::entry] if you want to insert *or* modify, /// perhaps only using the index for new entries with [`VacantEntry::shift_insert`]. + /// + /// # Examples + /// + /// ``` + /// use indexmap::IndexMap; + /// let mut map: IndexMap = ('a'..='z').map(|c| (c, ())).collect(); + /// + /// // The new key '*' goes exactly at the given index. + /// assert_eq!(map.get_index_of(&'*'), None); + /// assert_eq!(map.shift_insert(10, '*', ()), None); + /// assert_eq!(map.get_index_of(&'*'), Some(10)); + /// + /// // Moving the key 'a' up to 10 will shift others down, including the '*' that was at 10. + /// assert_eq!(map.shift_insert(10, 'a', ()), Some(())); + /// assert_eq!(map.get_index_of(&'a'), Some(10)); + /// assert_eq!(map.get_index_of(&'*'), Some(9)); + /// + /// // Moving the key 'z' down to 9 will shift others up, including the '*' that was at 9. + /// assert_eq!(map.shift_insert(9, 'z', ()), Some(())); + /// assert_eq!(map.get_index_of(&'z'), Some(9)); + /// assert_eq!(map.get_index_of(&'*'), Some(10)); + /// + /// // Existing keys can move to len-1 at most, but new keys can insert at the endpoint. + /// assert_eq!(map.len(), 27); + /// assert_eq!(map.shift_insert(map.len() - 1, '*', ()), Some(())); + /// assert_eq!(map.get_index_of(&'*'), Some(26)); + /// assert_eq!(map.shift_insert(map.len(), '+', ()), None); + /// assert_eq!(map.get_index_of(&'+'), Some(27)); + /// assert_eq!(map.len(), 28); + /// ``` + /// + /// ```should_panic + /// use indexmap::IndexMap; + /// let mut map: IndexMap = ('a'..='z').map(|c| (c, ())).collect(); + /// + /// // This is an invalid index for moving an existing key! + /// map.shift_insert(map.len(), 'a', ()); + /// ``` pub fn shift_insert(&mut self, index: usize, key: K, value: V) -> Option { let len = self.len(); match self.entry(key) { diff --git a/src/set.rs b/src/set.rs index c7b97502..250e31b3 100644 --- a/src/set.rs +++ b/src/set.rs @@ -396,6 +396,36 @@ where /// Valid indices are `0..=set.len()` (inclusive). /// /// Computes in **O(n)** time (average). + /// + /// # Examples + /// + /// ``` + /// use indexmap::IndexSet; + /// let mut set: IndexSet = ('a'..='z').collect(); + /// + /// // The new value '*' goes exactly at the given index. + /// assert_eq!(set.get_index_of(&'*'), None); + /// assert_eq!(set.insert_before(10, '*'), (10, true)); + /// assert_eq!(set.get_index_of(&'*'), Some(10)); + /// + /// // Moving the value 'a' up will shift others down, so this moves *before* 10 to index 9. + /// assert_eq!(set.insert_before(10, 'a'), (9, false)); + /// assert_eq!(set.get_index_of(&'a'), Some(9)); + /// assert_eq!(set.get_index_of(&'*'), Some(10)); + /// + /// // Moving the value 'z' down will shift others up, so this moves to exactly 10. + /// assert_eq!(set.insert_before(10, 'z'), (10, false)); + /// assert_eq!(set.get_index_of(&'z'), Some(10)); + /// assert_eq!(set.get_index_of(&'*'), Some(11)); + /// + /// // Moving or inserting before the endpoint is also valid. + /// assert_eq!(set.len(), 27); + /// assert_eq!(set.insert_before(set.len(), '*'), (26, false)); + /// assert_eq!(set.get_index_of(&'*'), Some(26)); + /// assert_eq!(set.insert_before(set.len(), '+'), (27, true)); + /// assert_eq!(set.get_index_of(&'+'), Some(27)); + /// assert_eq!(set.len(), 28); + /// ``` pub fn insert_before(&mut self, index: usize, value: T) -> (usize, bool) { let (index, existing) = self.map.insert_before(index, value, ()); (index, existing.is_none()) @@ -415,6 +445,44 @@ where /// `0..=set.len()` (inclusive) when inserting a new value. /// /// Computes in **O(n)** time (average). + /// + /// # Examples + /// + /// ``` + /// use indexmap::IndexSet; + /// let mut set: IndexSet = ('a'..='z').collect(); + /// + /// // The new value '*' goes exactly at the given index. + /// assert_eq!(set.get_index_of(&'*'), None); + /// assert_eq!(set.shift_insert(10, '*'), true); + /// assert_eq!(set.get_index_of(&'*'), Some(10)); + /// + /// // Moving the value 'a' up to 10 will shift others down, including the '*' that was at 10. + /// assert_eq!(set.shift_insert(10, 'a'), false); + /// assert_eq!(set.get_index_of(&'a'), Some(10)); + /// assert_eq!(set.get_index_of(&'*'), Some(9)); + /// + /// // Moving the value 'z' down to 9 will shift others up, including the '*' that was at 9. + /// assert_eq!(set.shift_insert(9, 'z'), false); + /// assert_eq!(set.get_index_of(&'z'), Some(9)); + /// assert_eq!(set.get_index_of(&'*'), Some(10)); + /// + /// // Existing values can move to len-1 at most, but new values can insert at the endpoint. + /// assert_eq!(set.len(), 27); + /// assert_eq!(set.shift_insert(set.len() - 1, '*'), false); + /// assert_eq!(set.get_index_of(&'*'), Some(26)); + /// assert_eq!(set.shift_insert(set.len(), '+'), true); + /// assert_eq!(set.get_index_of(&'+'), Some(27)); + /// assert_eq!(set.len(), 28); + /// ``` + /// + /// ```should_panic + /// use indexmap::IndexSet; + /// let mut set: IndexSet = ('a'..='z').collect(); + /// + /// // This is an invalid index for moving an existing value! + /// set.shift_insert(set.len(), 'a'); + /// ``` pub fn shift_insert(&mut self, index: usize, value: T) -> bool { self.map.shift_insert(index, value, ()).is_none() }