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.

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

Migrate unit tests of btree collections to their native breeding ground #75531

Merged
merged 2 commits into from
Aug 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions library/alloc/src/collections/btree/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3024,3 +3024,6 @@ impl<K: Ord, V, I: Iterator<Item = (K, V)>> Iterator for MergeIter<K, V, I> {
}
}
}

#[cfg(test)]
mod tests;
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
use std::collections::btree_map::Entry::{Occupied, Vacant};
use std::collections::BTreeMap;
use crate::boxed::Box;
use crate::collections::btree_map::Entry::{Occupied, Vacant};
use crate::collections::BTreeMap;
use crate::fmt::Debug;
use crate::rc::Rc;
use crate::string::String;
use crate::string::ToString;
use crate::vec::Vec;
use std::convert::TryFrom;
use std::fmt::Debug;
use std::iter::FromIterator;
use std::mem;
use std::ops::Bound::{self, Excluded, Included, Unbounded};
use std::ops::RangeBounds;
use std::panic::{catch_unwind, AssertUnwindSafe};
use std::rc::Rc;
use std::sync::atomic::{AtomicUsize, Ordering};

use super::DeterministicRng;
use super::super::DeterministicRng;

// Value of node::CAPACITY, thus capacity of a tree with a single level,
// i.e. a tree who's root is a leaf node at height 0.
Expand Down
27 changes: 27 additions & 0 deletions library/alloc/src/collections/btree/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,30 @@ pub unsafe fn unwrap_unchecked<T>(val: Option<T>) -> T {
}
})
}

#[cfg(test)]
/// XorShiftRng
struct DeterministicRng {
x: u32,
y: u32,
z: u32,
w: u32,
}

#[cfg(test)]
impl DeterministicRng {
fn new() -> Self {
DeterministicRng { x: 0x193a6754, y: 0xa8a7d469, z: 0x97830e05, w: 0x113ba7bb }
}

fn next(&mut self) -> u32 {
let x = self.x;
let t = x ^ (x << 11);
self.x = self.y;
self.y = self.z;
self.z = self.w;
let w_ = self.w;
self.w = w_ ^ (w_ >> 19) ^ (t ^ (t >> 8));
self.w
}
}
42 changes: 11 additions & 31 deletions library/alloc/src/collections/btree/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ use crate::boxed::Box;
const B: usize = 6;
pub const MIN_LEN: usize = B - 1;
pub const CAPACITY: usize = 2 * B - 1;
const KV_IDX_CENTER: usize = B - 1;
const EDGE_IDX_LEFT_OF_CENTER: usize = B - 1;
const EDGE_IDX_RIGHT_OF_CENTER: usize = B;

/// The underlying representation of leaf nodes.
#[repr(C)]
Expand Down Expand Up @@ -834,38 +837,12 @@ enum InsertionPlace {
fn splitpoint(edge_idx: usize) -> (usize, InsertionPlace) {
debug_assert!(edge_idx <= CAPACITY);
// Rust issue #74834 tries to explain these symmetric rules.
let middle_kv_idx;
let insertion;
if edge_idx <= B - 2 {
middle_kv_idx = B - 2;
insertion = InsertionPlace::Left(edge_idx);
} else if edge_idx == B - 1 {
middle_kv_idx = B - 1;
insertion = InsertionPlace::Left(edge_idx);
} else if edge_idx == B {
middle_kv_idx = B - 1;
insertion = InsertionPlace::Right(0);
} else {
middle_kv_idx = B;
let new_edge_idx = edge_idx - (B + 1);
insertion = InsertionPlace::Right(new_edge_idx);
}
let mut left_len = middle_kv_idx;
let mut right_len = CAPACITY - middle_kv_idx - 1;
match insertion {
InsertionPlace::Left(edge_idx) => {
debug_assert!(edge_idx <= left_len);
left_len += 1;
}
InsertionPlace::Right(edge_idx) => {
debug_assert!(edge_idx <= right_len);
right_len += 1;
}
match edge_idx {
0..EDGE_IDX_LEFT_OF_CENTER => (KV_IDX_CENTER - 1, InsertionPlace::Left(edge_idx)),
EDGE_IDX_LEFT_OF_CENTER => (KV_IDX_CENTER, InsertionPlace::Left(edge_idx)),
EDGE_IDX_RIGHT_OF_CENTER => (KV_IDX_CENTER, InsertionPlace::Right(0)),
_ => (KV_IDX_CENTER + 1, InsertionPlace::Right(edge_idx - (KV_IDX_CENTER + 1 + 1))),
}
debug_assert!(left_len >= MIN_LEN);
debug_assert!(right_len >= MIN_LEN);
debug_assert!(left_len + right_len == CAPACITY);
(middle_kv_idx, insertion)
}

impl<'a, K, V, NodeType> Handle<NodeRef<marker::Mut<'a>, K, V, NodeType>, marker::Edge> {
Expand Down Expand Up @@ -1590,3 +1567,6 @@ unsafe fn slice_remove<T>(slice: &mut [T], idx: usize) -> T {
ret
}
}

#[cfg(test)]
mod tests;
25 changes: 25 additions & 0 deletions library/alloc/src/collections/btree/node/tests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
use super::*;

#[test]
fn test_splitpoint() {
for idx in 0..=CAPACITY {
let (middle_kv_idx, insertion) = splitpoint(idx);

// Simulate performing the split:
let mut left_len = middle_kv_idx;
let mut right_len = CAPACITY - middle_kv_idx - 1;
match insertion {
InsertionPlace::Left(edge_idx) => {
assert!(edge_idx <= left_len);
left_len += 1;
}
InsertionPlace::Right(edge_idx) => {
assert!(edge_idx <= right_len);
right_len += 1;
}
}
assert!(left_len >= MIN_LEN);
assert!(right_len >= MIN_LEN);
assert!(left_len + right_len == CAPACITY);
}
}
3 changes: 3 additions & 0 deletions library/alloc/src/collections/btree/set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1572,3 +1572,6 @@ impl<'a, T: Ord> Iterator for Union<'a, T> {

#[stable(feature = "fused", since = "1.26.0")]
impl<T: Ord> FusedIterator for Union<'_, T> {}

#[cfg(test)]
mod tests;
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
use std::collections::BTreeSet;
use crate::collections::BTreeSet;
use crate::vec::Vec;
use std::iter::FromIterator;
use std::panic::{catch_unwind, AssertUnwindSafe};
use std::sync::atomic::{AtomicU32, Ordering};

use super::DeterministicRng;
use super::super::DeterministicRng;

#[test]
fn test_clone_eq() {
Expand All @@ -15,24 +16,6 @@ fn test_clone_eq() {
assert_eq!(m.clone(), m);
}

#[test]
fn test_hash() {
use crate::hash;

let mut x = BTreeSet::new();
let mut y = BTreeSet::new();

x.insert(1);
x.insert(2);
x.insert(3);

y.insert(3);
y.insert(2);
y.insert(1);

assert_eq!(hash(&x), hash(&y));
}

#[test]
fn test_iter_min_max() {
let mut a = BTreeSet::new();
Expand Down
4 changes: 4 additions & 0 deletions library/alloc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@
#![feature(arbitrary_self_types)]
#![feature(box_patterns)]
#![feature(box_syntax)]
#![feature(btree_drain_filter)]
#![feature(cfg_sanitize)]
#![feature(cfg_target_has_atomic)]
#![feature(coerce_unsized)]
Expand All @@ -93,6 +94,7 @@
#![feature(container_error_extra)]
#![feature(dropck_eyepatch)]
#![feature(exact_size_is_empty)]
#![feature(exclusive_range_pattern)]
#![feature(extend_one)]
#![feature(fmt_internals)]
#![feature(fn_traits)]
Expand All @@ -101,6 +103,8 @@
#![feature(lang_items)]
#![feature(layout_for_ptr)]
#![feature(libc)]
#![feature(map_first_last)]
#![feature(map_into_keys_values)]
#![feature(negative_impls)]
#![feature(new_uninit)]
#![feature(nll)]
Expand Down
27 changes: 0 additions & 27 deletions library/alloc/tests/btree/mod.rs

This file was deleted.

19 changes: 19 additions & 0 deletions library/alloc/tests/btree_set_hash.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
use std::collections::BTreeSet;

#[test]
fn test_hash() {
use crate::hash;

let mut x = BTreeSet::new();
let mut y = BTreeSet::new();

x.insert(1);
x.insert(2);
x.insert(3);

y.insert(3);
y.insert(2);
y.insert(1);

assert_eq!(hash(&x), hash(&y));
Copy link
Member

Choose a reason for hiding this comment

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

Can you say more about why this didn't migrate? (E.g. the error log)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hash function is defined in the integration test itself and uses the hashmap crate or whatever contains it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't really look into what DefaultHasher is, it looks foreign enough for me to say it's an integration test.

Copy link
Contributor Author

@ssomers ssomers Aug 14, 2020

Choose a reason for hiding this comment

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

Or rather, to say it's not a btree unit test. In my book, it could be a hashmap unit test, built on top of btree.

Copy link
Member

Choose a reason for hiding this comment

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

hm okay, seems fine to leave it external, though I suspect we could internalize it without too much trouble if we really wanted to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see: it tests the default Hash implementation on BTreeSet. Why not test the non-default Hash implementation on BTreeMap for a change?

}
5 changes: 1 addition & 4 deletions library/alloc/tests/lib.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
#![feature(allocator_api)]
#![feature(box_syntax)]
#![feature(btree_drain_filter)]
#![feature(drain_filter)]
#![feature(exact_size_is_empty)]
#![feature(map_first_last)]
#![feature(map_into_keys_values)]
#![feature(new_uninit)]
#![feature(pattern)]
#![feature(str_split_once)]
Expand All @@ -25,7 +22,7 @@ mod arc;
mod binary_heap;
mod borrow;
mod boxed;
mod btree;
mod btree_set_hash;
mod cow_str;
mod fmt;
mod heap;
Expand Down