Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Improve BoundedVec API (extracted from #10195) #10656

Merged
gavofyork merged 6 commits into from
Jan 17, 2022
Merged

Improve BoundedVec API (extracted from #10195) #10656

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e61b91b
Gav wrote this code in pull #10195. Extracting to simplify that PR.
gavofyork Jan 14, 2022
a3e273a
fix potential panics
shawntabrizi Jan 14, 2022
0ed947c
prevent panics in slide
shawntabrizi Jan 14, 2022
ec062ed
update doc
shawntabrizi Jan 14, 2022
5898129
fmt
shawntabrizi Jan 14, 2022
29305df
Update frame/support/src/storage/bounded_vec.rs
shawntabrizi Jan 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
218 changes: 216 additions & 2 deletions frame/support/src/storage/bounded_vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,16 @@ impl<T, S> BoundedVec<T, S> {
) -> Option<&mut <I as SliceIndex<[T]>>::Output> {
self.0.get_mut(index)
}

/// Exactly the same semantics as [`Vec::truncate`].
pub fn truncate(&mut self, s: usize) {
self.0.truncate(s);
}

/// Exactly the same semantics as [`Vec::pop`].
pub fn pop(&mut self) -> Option<T> {
self.0.pop()
}
}

impl<T, S: Get<u32>> From<BoundedVec<T, S>> for Vec<T> {
Expand Down Expand Up @@ -176,6 +186,115 @@ impl<T, S: Get<u32>> BoundedVec<T, S> {
S::get() as usize
}

/// Forces the insertion of `s` into `self` retaining all items with index at least `index`.
///
/// If `index == 0` and `self.len() == Self::bound()`, then this is a no-op.
///
/// If `Self::bound() < index` or `self.len() < index`, then this is also a no-op.
///
/// Returns `true` if the item was inserted.
pub fn force_insert_keep_right(&mut self, index: usize, element: T) -> bool {
// Check against panics.
if Self::bound() < index || self.len() < index {
return false
}
if self.len() < Self::bound() {
// Cannot panic since self.len() >= index;
self.0.insert(index, element);
} else {
if index == 0 {
return false
}
self[0] = element;
Comment thread
shawntabrizi marked this conversation as resolved.
// `[0..index] cannot panic since self.len() >= index.
// `rotate_left(1)` cannot panic because there is at least 1 element.
self[0..index].rotate_left(1);
}
true
}

/// Forces the insertion of `s` into `self` retaining all items with index at most `index`.
///
/// If `index == Self::bound()` and `self.len() == Self::bound()`, then this is a no-op.
///
/// If `Self::bound() < index` or `self.len() < index`, then this is also a no-op.
///
/// Returns `true` if the item was inserted.
pub fn force_insert_keep_left(&mut self, index: usize, element: T) -> bool {
// Check against panics.
if Self::bound() < index || self.len() < index {
return false
}
// Noop condition.
if Self::bound() == index && self.len() <= Self::bound() {
return false
}
// Cannot panic since self.len() >= index;
self.0.insert(index, element);
self.0.truncate(Self::bound());
true
}

/// Move the position of an item from one location to another in the slice.
///
/// Except for the item being moved, the order of the slice remains the same.
///
/// - `index` is the location of the item to be moved.
/// - `insert_position` is the index of the item in the slice which should *immediately follow*
/// the item which is being moved.
///
/// Returns `true` of the operation was successful, otherwise `false` if a noop.
pub fn slide(&mut self, index: usize, insert_position: usize) -> bool {
// Check against panics.
if self.len() <= index || self.len() < insert_position || index == usize::MAX {
return false
}
// Noop conditions.
if index == insert_position || index + 1 == insert_position {
return false
}
if insert_position < index && index < self.len() {
// --- --- --- === === === === @@@ --- --- ---
// ^-- N ^O^
// ...
// /-----<<<-----\
// --- --- --- === === === === @@@ --- --- ---
// >>> >>> >>> >>>
// ...
// --- --- --- @@@ === === === === --- --- ---
// ^N^
self[insert_position..index + 1].rotate_right(1);
return true
} else if insert_position > 0 && index + 1 < insert_position {
// Note that the apparent asymmetry of these two branches is due to the
// fact that the "new" position is the position to be inserted *before*.
// --- --- --- @@@ === === === === --- --- ---
// ^O^ ^-- N
// ...
// /----->>>-----\
// --- --- --- @@@ === === === === --- --- ---
// <<< <<< <<< <<<
// ...
// --- --- --- === === === === @@@ --- --- ---
// ^N^
self[index..insert_position].rotate_left(1);
return true
}

debug_assert!(false, "all noop conditions should have been covered above");
Copy link
Copy Markdown
Contributor

@KiChjang KiChjang Jan 14, 2022

Choose a reason for hiding this comment

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

Wonder if we can try fuzzing this function to ensure that this assertion is never hit.

Copy link
Copy Markdown
Member

@gavofyork gavofyork Jan 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

no need to fuzz it; you can just prove it logically.

false
}

/// Forces the insertion of `s` into `self` truncating first if necessary.
///
/// Infallible, but if the bound is zero, then it's a no-op.
pub fn force_push(&mut self, element: T) {
if Self::bound() > 0 {
self.0.truncate(Self::bound() as usize - 1);
self.0.push(element);
}
}

/// Same as `Vec::resize`, but if `size` is more than [`Self::bound`], then [`Self::bound`] is
/// used.
pub fn bounded_resize(&mut self, size: usize, value: T)
Expand Down Expand Up @@ -397,8 +516,7 @@ where
#[cfg(test)]
pub mod test {
use super::*;
use crate::Twox128;
use frame_support::traits::ConstU32;
use crate::{traits::ConstU32, Twox128};
use sp_io::TestExternalities;

crate::generate_storage_alias! { Prefix, Foo => Value<BoundedVec<u32, ConstU32<7>>> }
Expand All @@ -408,6 +526,102 @@ pub mod test {
FooDoubleMap => DoubleMap<(u32, Twox128), (u32, Twox128), BoundedVec<u32, ConstU32<7>>>
}

#[test]
fn slide_works() {
let mut b: BoundedVec<u32, ConstU32<6>> = vec![0, 1, 2, 3, 4, 5].try_into().unwrap();
assert!(b.slide(1, 5));
assert_eq!(*b, vec![0, 2, 3, 4, 1, 5]);
assert!(b.slide(4, 0));
assert_eq!(*b, vec![1, 0, 2, 3, 4, 5]);
assert!(b.slide(0, 2));
assert_eq!(*b, vec![0, 1, 2, 3, 4, 5]);
assert!(b.slide(1, 6));
assert_eq!(*b, vec![0, 2, 3, 4, 5, 1]);
assert!(b.slide(0, 6));
assert_eq!(*b, vec![2, 3, 4, 5, 1, 0]);
assert!(b.slide(5, 0));
assert_eq!(*b, vec![0, 2, 3, 4, 5, 1]);
assert!(!b.slide(6, 0));
assert!(!b.slide(7, 0));
assert_eq!(*b, vec![0, 2, 3, 4, 5, 1]);

let mut c: BoundedVec<u32, ConstU32<6>> = vec![0, 1, 2].try_into().unwrap();
assert!(!c.slide(1, 5));
assert_eq!(*c, vec![0, 1, 2]);
assert!(!c.slide(4, 0));
assert_eq!(*c, vec![0, 1, 2]);
assert!(!c.slide(3, 0));
assert_eq!(*c, vec![0, 1, 2]);
assert!(c.slide(2, 0));
assert_eq!(*c, vec![2, 0, 1]);
}

#[test]
fn slide_noops_work() {
let mut b: BoundedVec<u32, ConstU32<6>> = vec![0, 1, 2, 3, 4, 5].try_into().unwrap();
assert!(!b.slide(3, 3));
assert_eq!(*b, vec![0, 1, 2, 3, 4, 5]);
assert!(!b.slide(3, 4));
assert_eq!(*b, vec![0, 1, 2, 3, 4, 5]);
}

#[test]
fn force_insert_keep_left_works() {
let mut b: BoundedVec<u32, ConstU32<4>> = vec![].try_into().unwrap();
assert!(!b.force_insert_keep_left(1, 10));
assert!(b.is_empty());

assert!(b.force_insert_keep_left(0, 30));
assert!(b.force_insert_keep_left(0, 10));
assert!(b.force_insert_keep_left(1, 20));
assert!(b.force_insert_keep_left(3, 40));
assert_eq!(*b, vec![10, 20, 30, 40]);
// at capacity.
assert!(!b.force_insert_keep_left(4, 41));
assert_eq!(*b, vec![10, 20, 30, 40]);
assert!(b.force_insert_keep_left(3, 31));
assert_eq!(*b, vec![10, 20, 30, 31]);
assert!(b.force_insert_keep_left(1, 11));
assert_eq!(*b, vec![10, 11, 20, 30]);
assert!(b.force_insert_keep_left(0, 1));
assert_eq!(*b, vec![1, 10, 11, 20]);

let mut z: BoundedVec<u32, ConstU32<0>> = vec![].try_into().unwrap();
assert!(z.is_empty());
assert!(!z.force_insert_keep_left(0, 10));
assert!(z.is_empty());
}

#[test]
fn force_insert_keep_right_works() {
let mut b: BoundedVec<u32, ConstU32<4>> = vec![].try_into().unwrap();
assert!(!b.force_insert_keep_right(1, 10));
assert!(b.is_empty());

assert!(b.force_insert_keep_right(0, 30));
assert!(b.force_insert_keep_right(0, 10));
assert!(b.force_insert_keep_right(1, 20));
assert!(b.force_insert_keep_right(3, 40));
assert_eq!(*b, vec![10, 20, 30, 40]);
// at capacity.
assert!(!b.force_insert_keep_right(0, 0));
assert_eq!(*b, vec![10, 20, 30, 40]);
assert!(b.force_insert_keep_right(1, 11));
assert_eq!(*b, vec![11, 20, 30, 40]);
assert!(b.force_insert_keep_right(3, 31));
assert_eq!(*b, vec![20, 30, 31, 40]);
assert!(b.force_insert_keep_right(4, 41));
assert_eq!(*b, vec![30, 31, 40, 41]);

assert!(!b.force_insert_keep_right(5, 69));
assert_eq!(*b, vec![30, 31, 40, 41]);

let mut z: BoundedVec<u32, ConstU32<0>> = vec![].try_into().unwrap();
assert!(z.is_empty());
assert!(!z.force_insert_keep_right(0, 10));
assert!(z.is_empty());
}

#[test]
fn try_append_is_correct() {
assert_eq!(BoundedVec::<u32, ConstU32<7>>::bound(), 7);
Expand Down