Skip to content
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.

Sign up for GitHub

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

Implement insert for RingBuf #19569

Merged
merged 1 commit into from
Dec 12, 2014
Merged

Implement insert for RingBuf #19569

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
268 changes: 267 additions & 1 deletion src/libcollections/ring_buf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,21 @@ impl<T> RingBuf<T> {
/// Returns the index in the underlying buffer for a given logical element index.
#[inline]
fn wrap_index(&self, idx: uint) -> uint { wrap_index(idx, self.cap) }

/// Copies a contiguous block of memory len long from src to dst
#[inline]
fn copy(&self, dst: uint, src: uint, len: uint) {
unsafe {
debug_assert!(dst + len <= self.cap, "dst={} src={} len={} cap={}", dst, src, len,
self.cap);
debug_assert!(src + len <= self.cap, "dst={} src={} len={} cap={}", dst, src, len,
self.cap);
ptr::copy_memory(
self.ptr.offset(dst as int),
self.ptr.offset(src as int) as *const T,
len);
}
}
}

impl<T> RingBuf<T> {
Expand Down Expand Up @@ -648,6 +663,213 @@ impl<T> RingBuf<T> {
unsafe { Some(self.buffer_read(head)) }
}
}

/// Inserts an element at position `i` within the ringbuf. Whichever
/// end is closer to the insertion point will be moved to make room,
/// and all the affected elements will be moved to new positions.
///
/// # Panics
///
/// Panics if `i` is greater than ringbuf's length
///
/// # Example
/// ```rust
/// use std::collections::RingBuf;
///
/// let mut buf = RingBuf::new();
/// buf.push_back(10i);
/// buf.push_back(12);
/// buf.insert(1,11);
/// assert_eq!(Some(&11), buf.get(1));
/// ```
pub fn insert(&mut self, i: uint, t: T) {
assert!(i <= self.len(), "index out of bounds");
if self.is_full() {
self.reserve(1);
debug_assert!(!self.is_full());
}

// Move the least number of elements in the ring buffer and insert
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be useful to establish here that there are three main cases:

  • elements are contiguous
  • elements are discontiguous - and index is in the tail half
  • elements are discontiguous - and index is in the head half

With each case subdivided into:

  • index is closer to head
  • index is closer to tail

// the given object
//
// At most len/2 - 1 elements will be moved. O(min(n, n-i))
//
// There are three main cases:
// Elements are contiguous
// - special case when tail is 0
// Elements are discontiguous and the insert is in the tail section
// Elements are discontiguous and the insert is in the head section
//
// For each of those there are two more cases:
// Insert is closer to tail
// Insert is closer to head
//
// Key: H - self.head
// T - self.tail
// o - Valid element
// I - Insertion element
// A - The element that should be after the insertion point
// M - Indicates element was moved

let idx = self.wrap_index(self.tail + i);

let distance_to_tail = i;
let distance_to_head = self.len() - i;

let contiguous = self.tail <= self.head;

match (contiguous, distance_to_tail <= distance_to_head, idx >= self.tail) {
(true, true, _) if i == 0 => {
// push_front
//
// T
// I H
// [A o o o o o o . . . . . . . . .]
//
// H T
// [A o o o o o o o . . . . . I]
//

self.tail = self.wrap_index(self.tail - 1);
},
(true, true, _) => {
// contiguous, insert closer to tail:
//
// T I H
// [. . . o o A o o o o . . . . . .]
//
// T H
// [. . o o I A o o o o . . . . . .]
// M M
//
// contiguous, insert closer to tail and tail is 0:
//
//
// T I H
// [o o A o o o o . . . . . . . . .]
//
// H T
// [o I A o o o o o . . . . . . . o]
// M M

let old_tail = self.tail;
self.tail = self.wrap_index(self.tail - 1);

self.copy(self.tail, old_tail, 1);
self.copy(old_tail, old_tail + 1, i);
},
(true, false, _) => {
// contiguous, insert closer to head:
//
// T I H
// [. . . o o o o A o o . . . . . .]
//
// T H
// [. . . o o o o I A o o . . . . .]
// M M M

let old_head = self.head;
self.head = self.wrap_index(self.head + 1);
self.copy(idx + 1, idx, old_head - idx);
},
(false, true, true) => {
// discontiguous, tail section, insert closer to tail:
//
// H T I
// [o o o o o o . . . . . o o A o o]
//
// H T
// [o o o o o o . . . . o o I A o o]
// M M

let old_tail = self.tail;
self.tail = self.tail - 1;
self.copy(self.tail, old_tail, i);
},
(false, false, true) => {
// discontiguous, tail section, insert closer to head:
//
// H T I
// [o o . . . . . . . o o o o o A o]
//
// H T
// [o o o . . . . . . o o o o o I A]
// M M M M

let old_head = self.head;
self.head = self.head + 1;

// copy elements up to new head
self.copy(1, 0, old_head);

// copy last element into empty spot at bottom of buffer
self.copy(0, self.cap - 1, 1);

// move elements from idx to end forward not including ^ element
self.copy(idx + 1, idx, self.cap - 1 - idx);
},
(false, true, false) if idx == 0 => {
// discontiguous, head section, insert is closer to tail,
// and is at index zero in the internal buffer:
//
// I H T
// [A o o o o o o o o o . . . o o o]
//
// H T
// [A o o o o o o o o o . . o o o I]
// M M M

let old_tail = self.tail;
self.tail = self.tail - 1;
// copy elements up to new tail
self.copy(old_tail - 1, old_tail, i);

// copy last element into empty spot at bottom of buffer
self.copy(self.cap - 1, 0, 1);
},
(false, true, false) => {
// discontiguous, head section, insert closer to tail:
//
// I H T
// [o o o A o o o o o o . . . o o o]
//
// H T
// [o o I A o o o o o o . . o o o o]
// M M M M M M

let old_tail = self.tail;
self.tail = self.tail - 1;
// copy elements up to new tail
self.copy(old_tail - 1, old_tail, i);

// copy last element into empty spot at bottom of buffer
self.copy(self.cap - 1, 0, 1);

// move elements from idx-1 to end forward not including ^ element
self.copy(0, 1, idx - 1);
}
(false, false, false) => {
// discontiguous, head section, insert closer to head:
//
// I H T
// [o o o o A o o . . . . . . o o o]
//
// H T
// [o o o o I A o o . . . . . o o o]
// M M M

let old_head = self.head;
self.head = self.head + 1;
self.copy(idx + 1, idx, old_head - idx);
}
}

// tail might've been changed so we need to recalculate
let new_idx = self.wrap_index(self.tail + i);
unsafe {
self.buffer_write(new_idx, t);
}
}
}

/// Returns the index in the underlying buffer for a given logical element index.
Expand Down Expand Up @@ -878,6 +1100,7 @@ impl<T: fmt::Show> fmt::Show for RingBuf<T> {

#[cfg(test)]
mod tests {
use core::iter;
use self::Taggy::*;
use self::Taggypar::*;
use std::fmt::Show;
Expand Down Expand Up @@ -1102,7 +1325,6 @@ mod tests {
})
}


#[deriving(Clone, PartialEq, Show)]
enum Taggy {
One(int),
Expand Down Expand Up @@ -1666,4 +1888,48 @@ mod tests {
assert_eq!(ring.get_mut(1), Some(&mut 2));
assert_eq!(ring.get_mut(2), None);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to see a more comprehensive test here. I'm thinking something along the lines of: for every possible head and tail position in a 64-element ringbuf and every valid insertion position in that range; create the ringbuf, try the insertion, then validate that the contents meet expectations.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's quite a few 0-len, 1-len and cap-2-len cases to handle as well, I imagine.

If we're going full-blown, I'd favour something like testing every single valid ringbuf configuration for cap = 8. All lens (0 to 7), and all tail positions (0 to 7). Only 64 tests, not too bad.

Shouldn't be too bad to set up. The test modul can touch the ringbuf's privates. Allocate a ringbuf with_capacity(7). Get the capacity (may be larger than 7, but should still be pow-of-2-less-1), do a doubly-nested for-loop from 0 to that using the test modules access to the RIngBuf's privates to set tail manually, and fill in the rest with push_back's.

Copy link
Contributor

Choose a reason for hiding this comment

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

64^3-ish tests ;) all lens, tail positions, and valid insertion points.

#[test]
fn test_insert() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice! confidence++

// This test checks that every single combination of tail position, length, and
// insertion position is tested. Capacity 7 should be large enough to cover every case.

let mut tester = RingBuf::with_capacity(7);
// can't guarantee we got 7, so have to get what we got.
// 7 would be great, but we will definitely get 2^k - 1, for k >= 3, or else
// this test isn't covering what it wants to
let cap = tester.capacity();


// len is the length *after* insertion
for len in range(1, cap) {
// 0, 1, 2, .., len - 1
let expected = iter::count(0, 1).take(len).collect();
for tail_pos in range(0, cap) {
for to_insert in range(0, len) {
tester.tail = tail_pos;
tester.head = tail_pos;
for i in range(0, len) {
if i != to_insert {
tester.push_back(i);
}
}
tester.insert(to_insert, to_insert);
assert_eq!(tester, expected);
}
}
}
}

#[test]
fn test_front() {
let mut ring = RingBuf::new();
ring.push_back(10i);
ring.push_back(20i);
assert_eq!(ring.front(), Some(&10));
ring.pop_front();
assert_eq!(ring.front(), Some(&20));
ring.pop_front();
assert_eq!(ring.front(), None);
}
}