Skip to content

Commit 6ebd62b

Browse files
PazzazPazzaz
committed
Make VecDeque append safer and easier to understand
1 parent 9f1fdec commit 6ebd62b

File tree

1 file changed

+131
-109
lines changed

1 file changed

+131
-109
lines changed

‎src/liballoc/collections/vec_deque.rs

Lines changed: 131 additions & 109 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,20 @@ impl<T> VecDeque<T> {
202202
len);
203203
}
204204

205+
/// Returns a pair of slices which contain the contents of the buffer not used by the VecDeque.
206+
#[inline]
207+
unsafe fn unused_as_mut_slices<'a>(&'a mut self) -> (&'a mut [T], &'a mut [T]) {
208+
let head = self.head;
209+
let tail = self.tail;
210+
let buf = self.buffer_as_mut_slice();
211+
if head == tail {
212+
let (before, after) = buf.split_at_mut(head);
213+
(after, before)
214+
} else {
215+
RingSlices::ring_slices(buf, tail, head)
216+
}
217+
}
218+
205219
/// Copies a potentially wrapping block of memory len long from src to dest.
206220
/// (abs(dst - src) + len) must be no larger than cap() (There must be at
207221
/// most one continuous overlapping region between src and dest).
@@ -1834,144 +1848,152 @@ impl<T> VecDeque<T> {
18341848
#[inline]
18351849
#[stable(feature = "append", since = "1.4.0")]
18361850
pub fn append(&mut self, other: &mut Self) {
1837-
// Copy from src[i1..i1 + len] to dst[i2..i2 + len].
1838-
// Does not check if the ranges are valid.
1839-
unsafe fn copy_part<T>(i1: usize, i2: usize, len: usize, src: &[T], dst: &mut [T]) {
1840-
debug_assert!(src.get(i1..i1 + len).is_some() && dst.get(i2..i2 + len).is_some());
1841-
ptr::copy_nonoverlapping(src.as_ptr().add(i1), dst.as_mut_ptr().add(i2), len);
1851+
// Copies all values from `src_slice` to the start of `dst_slice`.
1852+
unsafe fn copy_whole_slice<T>(src_slice: &[T], dst_slice: &mut [T]) {
1853+
let len = src_slice.len();
1854+
ptr::copy_nonoverlapping(src_slice.as_ptr(), dst_slice[..len].as_mut_ptr(), len);
18421855
}
18431856

18441857
let src_total = other.len();
18451858

18461859
// Guarantees there is space in `self` for `other`.
18471860
self.reserve(src_total);
18481861

1849-
self.head = {
1850-
let dst_start_1 = self.head;
1851-
let src_start_1 = other.tail;
1852-
let dst_wrap_point = self.cap();
1853-
let src_wrap_point = other.cap();
1854-
1855-
let dst = unsafe { self.buffer_as_mut_slice() };
1856-
let src = unsafe { other.buffer_as_slice() };
1862+
let new_head = {
1863+
let original_head = self.head;
18571864

1858-
let src_wraps = other.tail > other.head;
1859-
let dst_wraps = dst_start_1 + src_total > dst_wrap_point;
1865+
// The goal is to copy all values from `other` into `self`. To avoid any
1866+
// mismatch, all valid values in `other` are retrieved...
1867+
let (src_high, src_low) = other.as_slices();
1868+
// and unoccupied parts of self are retrieved.
1869+
let (dst_high, dst_low) = unsafe { self.unused_as_mut_slices() };
18601870

1861-
// When minimizing the amount of calls to `copy_part`, there are
1862-
// 6 different cases to handle. Whether src and/or dst wrap are 4
1863-
// combinations and there are 3 distinct cases when they both wrap.
1864-
// 6 = 3 + 1 + 1 + 1
1865-
match (src_wraps, dst_wraps) {
1871+
// Then all that is needed is to copy all values from
1872+
// src (src_high and src_low) to dst (dst_high and dst_low).
1873+
//
1874+
// other [o o o . . . . . o o o o]
1875+
// [5 6 7] [1 2 3 4]
1876+
// src_low src_high
1877+
//
1878+
// self [. . . . . . o o o o . .]
1879+
// [3 4 5 6 7 .] [1 2]
1880+
// dst_low dst_high
1881+
//
1882+
// Values are not copied one by one but as slices in `copy_whole_slice`.
1883+
// What slices are used depends on various properties of src and dst.
1884+
// There are 6 cases in total:
1885+
// 1. `src` and `dst` are contiguous
1886+
// 2. `src` is contiguous and `dst` is discontiguous
1887+
// 3. `src` is discontiguous and `dst` is contiguous
1888+
// 4. `src` and `dst` are discontiguous
1889+
// + src_high is smaller than dst_high
1890+
// 5. `src` and `dst` are discontiguous
1891+
// + dst_high is smaller than src_high
1892+
// 6. `src` and `dst` are discontiguous
1893+
// + dst_high is the same size as src_high
1894+
let src_contiguous = src_low.is_empty();
1895+
let dst_contiguous = dst_high.len() >= src_total;
1896+
match (src_contiguous, dst_contiguous) {
18661897
(true, true) => {
1867-
let dst_before_wrap = dst_wrap_point - dst_start_1;
1868-
let src_before_wrap = src_wrap_point - src_start_1;
1869-
1870-
if src_before_wrap < dst_before_wrap {
1871-
// src
1872-
// [o o o . . . . . . o o o]
1873-
// 2 3 3 1 1 1
1874-
//
1875-
// dst
1876-
// [. . . . . . o o . . . .]
1877-
// 3 3 H 1 1 1 2
1878-
let src_2 = dst_before_wrap - src_before_wrap;
1879-
let dst_start_2 = dst_start_1 + src_before_wrap;
1880-
let src_3 = src_total - dst_before_wrap;
1881-
1882-
unsafe {
1883-
copy_part(src_start_1, dst_start_1, src_before_wrap, src, dst);
1884-
copy_part(0, dst_start_2, src_2, src, dst);
1885-
copy_part(src_2, 0, src_3, src, dst);
1886-
}
1887-
src_3
1888-
} else if src_before_wrap > dst_before_wrap {
1889-
// src
1890-
// [o o o . . . . . o o o o]
1891-
// 3 3 3 1 1 2 2
1892-
//
1893-
// dst
1894-
// [. . . . . . o o o o . .]
1895-
// 2 2 3 3 3 H 1 1
1896-
let src_2 = src_before_wrap - dst_before_wrap;
1897-
let src_start_2 = src_start_1 + dst_before_wrap;
1898-
let src_3 = src_total - src_before_wrap;
1899-
1900-
unsafe {
1901-
copy_part(src_start_1, dst_start_1, dst_before_wrap, src, dst);
1902-
copy_part(src_start_2, 0, src_2, src, dst);
1903-
copy_part(0, src_2, src_3, src, dst);
1904-
}
1905-
src_2 + src_3
1906-
} else {
1907-
// src
1908-
// [o o . . . . . . . o o o]
1909-
// 2 2 1 1 1
1910-
//
1911-
// dst
1912-
// [. . . . . . . o o . . .]
1913-
// 2 2 H 1 1 1
1914-
let src_2 = src_total - src_before_wrap;
1898+
// 1.
1899+
// other [. . . o o o . . . . . .]
1900+
// [] [1 1 1]
1901+
//
1902+
// self [. o o o o o . . . . . .]
1903+
// [.] [1 1 1 . . .]
19151904

1916-
unsafe {
1917-
copy_part(src_start_1, dst_start_1, src_before_wrap, src, dst);
1918-
copy_part(0, 0, src_2, src, dst);
1919-
}
1920-
src_2
1905+
unsafe {
1906+
copy_whole_slice(src_high, dst_high);
19211907
}
1908+
original_head + src_total
19221909
}
1923-
(false, true) => {
1924-
// src
1925-
// [. . . o o o o o . . . .]
1926-
// 1 1 2 2 2
1910+
(true, false) => {
1911+
// 2.
1912+
// other [. . . o o o o o . . . .]
1913+
// [] [1 1 2 2 2]
19271914
//
1928-
// dst
1929-
// [. . . . . . . o o o . .]
1930-
// 2 2 2 H 1 1
1931-
let dst_1 = dst_wrap_point - dst_start_1;
1932-
let src_start_2 = src_start_1 + dst_1;
1933-
let dst_2 = src_total - dst_1;
1915+
// self [. . . . . . . o o o . .]
1916+
// [2 2 2 . . . .] [1 1]
19341917

1918+
let (src_1, src_2) = src_high.split_at(dst_high.len());
19351919
unsafe {
1936-
copy_part(src_start_1, dst_start_1, dst_1, src, dst);
1937-
copy_part(src_start_2, 0, dst_2, src, dst);
1920+
copy_whole_slice(src_1, dst_high);
1921+
copy_whole_slice(src_2, dst_low);
19381922
}
1939-
dst_2
1923+
src_total - dst_high.len()
19401924
}
1941-
(true, false) => {
1942-
// src
1943-
// [o o . . . . . . . o o o]
1944-
// 2 2 1 1 1
1925+
(false, true) => {
1926+
// 3.
1927+
// other [o o . . . . . . . o o o]
1928+
// [2 2] [1 1 1]
19451929
//
1946-
// dst
1947-
// [. o o . . . . . . . . .]
1948-
// 1 1 1 2 2 H
1949-
let src_1 = src_wrap_point - src_start_1;
1950-
let dst_start_2 = dst_start_1 + src_1;
1951-
let src_2 = src_total - src_1;
1930+
// self [. o o . . . . . . . . .]
1931+
// [.] [1 1 1 2 2 . . . .]
19521932

1933+
let (dst_1, dst_2) = dst_high.split_at_mut(src_high.len());
19531934
unsafe {
1954-
copy_part(src_start_1, dst_start_1, src_1, src, dst);
1955-
copy_part(0, dst_start_2, src_2, src, dst);
1935+
copy_whole_slice(src_high, dst_1);
1936+
copy_whole_slice(src_low, dst_2);
19561937
}
1957-
dst_start_1 + src_1 + src_2
1938+
original_head + src_total
19581939
}
19591940
(false, false) => {
1960-
// src
1961-
// [. . . o o o . . . . . .]
1962-
// 1 1 1
1963-
//
1964-
// dst
1965-
// [. o o o o o . . . . . .]
1966-
// 1 1 1 H
1967-
unsafe {
1968-
copy_part(src_start_1, dst_start_1, src_total, src, dst);
1941+
if src_high.len() < dst_high.len() {
1942+
// 4.
1943+
// other [o o o . . . . . . o o o]
1944+
// [2 3 3] [1 1 1]
1945+
//
1946+
// self [. . . . . . o o . . . .]
1947+
// [3 3 . . . .] [1 1 1 2]
1948+
1949+
let (dst_1, dst_2) = dst_high.split_at_mut(src_high.len());
1950+
let (src_2, src_3) = src_low.split_at(dst_2.len());
1951+
unsafe {
1952+
copy_whole_slice(src_high, dst_1);
1953+
copy_whole_slice(src_2, dst_2);
1954+
copy_whole_slice(src_3, dst_low);
1955+
}
1956+
src_3.len()
1957+
} else if src_high.len() > dst_high.len() {
1958+
// 5.
1959+
// other [o o o . . . . . o o o o]
1960+
// [3 3 3] [1 1 2 2]
1961+
//
1962+
// self [. . . . . . o o o o . .]
1963+
// [2 2 3 3 3 .] [1 1]
1964+
1965+
let (src_1, src_2) = src_high.split_at(dst_high.len());
1966+
let (dst_2, dst_3) = dst_low.split_at_mut(src_2.len());
1967+
unsafe {
1968+
copy_whole_slice(src_1, dst_high);
1969+
copy_whole_slice(src_2, dst_2);
1970+
copy_whole_slice(src_low, dst_3);
1971+
}
1972+
dst_2.len() + src_low.len()
1973+
} else {
1974+
// 6.
1975+
// other [o o . . . . . . . o o o]
1976+
// [2 2] [1 1 1]
1977+
//
1978+
// self [. . . . . . . o o . . .]
1979+
// [2 2 . . . . .] [1 1 1]
1980+
1981+
unsafe {
1982+
copy_whole_slice(src_high, dst_high);
1983+
copy_whole_slice(src_low, dst_low);
1984+
}
1985+
src_low.len()
19691986
}
1970-
dst_start_1 + src_total
19711987
}
19721988
}
19731989
};
1990+
1991+
// Up until this point we are in a bad state as some values
1992+
// exist in both `self` and `other`. To preserve panic safety
1993+
// it is important we clear the old values from `other`...
19741994
other.clear();
1995+
// and that we update `head` as the last step, making the values accessible in `self`.
1996+
self.head = new_head;
19751997
}
19761998

19771999
/// Retains only the elements specified by the predicate.

0 commit comments

Comments
 (0)