From ab734ca571950a02419262cd3c910db17e3fd8a2 Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Tue, 9 Dec 2014 21:47:10 -0800 Subject: [PATCH 1/3] bitv: correct build failures - Fix typos on Blocks and MutBlocks. - Use slice_to_mut() for creating blocks_mut(). - Deref the block parameter in get(). - Access nbits separately from mutating set in pop(). --- src/libcollections/bit.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/libcollections/bit.rs b/src/libcollections/bit.rs index 0bdc383492f3a..dd5613e867f31 100644 --- a/src/libcollections/bit.rs +++ b/src/libcollections/bit.rs @@ -87,8 +87,8 @@ use std::hash; use vec::Vec; -type Blocks<'a> = Cloned> -type MutBlocks<'a> MutItems<'a, u32>; +type Blocks<'a> = Cloned>; +type MutBlocks<'a> = MutItems<'a, u32>; type MatchWords<'a> = Chain>, Skip>>>>; // Take two BitV's, and return iterators of their words, where the shorter one @@ -199,7 +199,7 @@ impl Bitv { /// Iterator over mutable refs to the underlying blocks of data. fn blocks_mut(&mut self) -> MutBlocks { let blocks = blocks_for_bits(self.len()); - self.storage[..blocks].iter_mut() + self.storage.slice_to_mut(blocks).iter_mut() } /// Iterator over the underlying blocks of data @@ -336,7 +336,7 @@ impl Bitv { assert!(i < self.nbits); let w = i / u32::BITS; let b = i % u32::BITS; - self.storage.get(w).map(|block| + self.storage.get(w).map(|&block| (block & (1 << b)) != 0 ) } @@ -835,10 +835,11 @@ impl Bitv { if self.is_empty() { None } else { - let ret = self[self.nbits - 1]; + let i = self.nbits - 1; + let ret = self[i]; // Second rule of Bitv Club - self.set(self.nbits - 1, false); - self.nbits -= 1; + self.set(i, false); + self.nbits = i; Some(ret) } } From 2a6e5b11aa53866ce1a24049450dae3e5057c88e Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Tue, 9 Dec 2014 22:06:52 -0800 Subject: [PATCH 2/3] bitv: Fix all() for nbits that are multiples of u32::BITS The old logic would be ok with *either* 0 or all 1s in the last word, because it didn't compute a proper mask for the case where nbits is an exact multiple of u32::BITS. Add mask_for_bits() to compute this properly, and use it in all(). Add all/none assertions to most of the tests. Note in particular, the all-zero bitv in test_32_elements() was incorrectly all()==true before this patch. --- src/libcollections/bit.rs | 41 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/src/libcollections/bit.rs b/src/libcollections/bit.rs index dd5613e867f31..e8f2130596dbb 100644 --- a/src/libcollections/bit.rs +++ b/src/libcollections/bit.rs @@ -174,6 +174,12 @@ fn blocks_for_bits(bits: uint) -> uint { } +/// Computes the bitmask for the final word of the vector +fn mask_for_bits(bits: uint) -> u32 { + // Note especially that a perfect multiple of u32::BITS should mask all 1s. + !0u32 >> (u32::BITS - bits % u32::BITS) +} + impl Bitv { /// Applies the given operation to the blocks of self and other, and sets self to /// be the result. @@ -526,7 +532,7 @@ impl Bitv { last_word = elem; tmp == !0u32 // and then check the last one has enough ones - }) && (last_word == ((1 << self.nbits % u32::BITS) - 1) || last_word == !0u32) + }) && (last_word == mask_for_bits(self.nbits)) } /// Returns an iterator over the elements of the vector in order. @@ -1795,14 +1801,17 @@ mod bitv_test { let act = Bitv::new(); let exp = Vec::from_elem(0u, false); assert!(act.eq_vec(exp.as_slice())); + assert!(act.none() && act.all()); } #[test] fn test_1_element() { let mut act = Bitv::from_elem(1u, false); assert!(act.eq_vec(&[false])); + assert!(act.none() && !act.all()); act = Bitv::from_elem(1u, true); assert!(act.eq_vec(&[true])); + assert!(!act.none() && act.all()); } #[test] @@ -1811,6 +1820,7 @@ mod bitv_test { b.set(0, true); b.set(1, false); assert_eq!(b.to_string().as_slice(), "10"); + assert!(!b.none() && !b.all()); } #[test] @@ -1821,10 +1831,12 @@ mod bitv_test { act = Bitv::from_elem(10u, false); assert!((act.eq_vec( &[false, false, false, false, false, false, false, false, false, false]))); + assert!(act.none() && !act.all()); // all 1 act = Bitv::from_elem(10u, true); assert!((act.eq_vec(&[true, true, true, true, true, true, true, true, true, true]))); + assert!(!act.none() && act.all()); // mixed act = Bitv::from_elem(10u, false); @@ -1834,6 +1846,7 @@ mod bitv_test { act.set(3u, true); act.set(4u, true); assert!((act.eq_vec(&[true, true, true, true, true, false, false, false, false, false]))); + assert!(!act.none() && !act.all()); // mixed act = Bitv::from_elem(10u, false); @@ -1843,6 +1856,7 @@ mod bitv_test { act.set(8u, true); act.set(9u, true); assert!((act.eq_vec(&[false, false, false, false, false, true, true, true, true, true]))); + assert!(!act.none() && !act.all()); // mixed act = Bitv::from_elem(10u, false); @@ -1851,6 +1865,7 @@ mod bitv_test { act.set(6u, true); act.set(9u, true); assert!((act.eq_vec(&[true, false, false, true, false, false, true, false, false, true]))); + assert!(!act.none() && !act.all()); } #[test] @@ -1863,6 +1878,7 @@ mod bitv_test { &[false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false])); + assert!(act.none() && !act.all()); // all 1 act = Bitv::from_elem(31u, true); @@ -1870,6 +1886,7 @@ mod bitv_test { &[true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true])); + assert!(!act.none() && act.all()); // mixed act = Bitv::from_elem(31u, false); @@ -1885,6 +1902,7 @@ mod bitv_test { &[true, true, true, true, true, true, true, true, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false])); + assert!(!act.none() && !act.all()); // mixed act = Bitv::from_elem(31u, false); @@ -1900,6 +1918,7 @@ mod bitv_test { &[false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, true, true, true, true, true, true, true, true, false, false, false, false, false, false, false])); + assert!(!act.none() && !act.all()); // mixed act = Bitv::from_elem(31u, false); @@ -1914,6 +1933,7 @@ mod bitv_test { &[false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, true, true, true, true, true, true, true])); + assert!(!act.none() && !act.all()); // mixed act = Bitv::from_elem(31u, false); @@ -1924,6 +1944,7 @@ mod bitv_test { &[false, false, false, true, false, false, false, false, false, false, false, false, false, false, false, false, false, true, false, false, false, false, false, false, false, false, false, false, false, false, true])); + assert!(!act.none() && !act.all()); } #[test] @@ -1936,6 +1957,7 @@ mod bitv_test { &[false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false])); + assert!(act.none() && !act.all()); // all 1 act = Bitv::from_elem(32u, true); @@ -1943,6 +1965,7 @@ mod bitv_test { &[true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true])); + assert!(!act.none() && act.all()); // mixed act = Bitv::from_elem(32u, false); @@ -1958,6 +1981,7 @@ mod bitv_test { &[true, true, true, true, true, true, true, true, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false])); + assert!(!act.none() && !act.all()); // mixed act = Bitv::from_elem(32u, false); @@ -1973,6 +1997,7 @@ mod bitv_test { &[false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, true, true, true, true, true, true, true, true, false, false, false, false, false, false, false, false])); + assert!(!act.none() && !act.all()); // mixed act = Bitv::from_elem(32u, false); @@ -1988,6 +2013,7 @@ mod bitv_test { &[false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, true, true, true, true, true, true, true, true])); + assert!(!act.none() && !act.all()); // mixed act = Bitv::from_elem(32u, false); @@ -1999,6 +2025,7 @@ mod bitv_test { &[false, false, false, true, false, false, false, false, false, false, false, false, false, false, false, false, false, true, false, false, false, false, false, false, false, false, false, false, false, false, true, true])); + assert!(!act.none() && !act.all()); } #[test] @@ -2011,6 +2038,7 @@ mod bitv_test { &[false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false])); + assert!(act.none() && !act.all()); // all 1 act = Bitv::from_elem(33u, true); @@ -2018,6 +2046,7 @@ mod bitv_test { &[true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true])); + assert!(!act.none() && act.all()); // mixed act = Bitv::from_elem(33u, false); @@ -2033,6 +2062,7 @@ mod bitv_test { &[true, true, true, true, true, true, true, true, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false])); + assert!(!act.none() && !act.all()); // mixed act = Bitv::from_elem(33u, false); @@ -2048,6 +2078,7 @@ mod bitv_test { &[false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, true, true, true, true, true, true, true, true, false, false, false, false, false, false, false, false, false])); + assert!(!act.none() && !act.all()); // mixed act = Bitv::from_elem(33u, false); @@ -2063,6 +2094,7 @@ mod bitv_test { &[false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, true, true, true, true, true, true, true, true, false])); + assert!(!act.none() && !act.all()); // mixed act = Bitv::from_elem(33u, false); @@ -2075,6 +2107,7 @@ mod bitv_test { &[false, false, false, true, false, false, false, false, false, false, false, false, false, false, false, false, false, true, false, false, false, false, false, false, false, false, false, false, false, false, true, true, true])); + assert!(!act.none() && !act.all()); } #[test] @@ -2192,15 +2225,17 @@ mod bitv_test { #[test] fn test_small_clear() { let mut b = Bitv::from_elem(14, true); + assert!(!b.none() && b.all()); b.clear(); - assert!(b.none()); + assert!(b.none() && !b.all()); } #[test] fn test_big_clear() { let mut b = Bitv::from_elem(140, true); + assert!(!b.none() && b.all()); b.clear(); - assert!(b.none()); + assert!(b.none() && !b.all()); } #[test] From 0f70e1a85ad097fd7c4228d588879af21a9a2b3e Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Tue, 9 Dec 2014 22:11:36 -0800 Subject: [PATCH 3/3] bitv: reuse mask_for_bits() in grow() --- src/libcollections/bit.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libcollections/bit.rs b/src/libcollections/bit.rs index e8f2130596dbb..25d9fbee0a766 100644 --- a/src/libcollections/bit.rs +++ b/src/libcollections/bit.rs @@ -794,15 +794,15 @@ impl Bitv { let new_nblocks = blocks_for_bits(new_nbits); let full_value = if value { !0 } else { 0 }; - // Correct the old tail word + // Correct the old tail word, setting or clearing formerly unused bits let old_last_word = blocks_for_bits(self.nbits) - 1; if self.nbits % u32::BITS > 0 { - let overhang = self.nbits % u32::BITS; // # of already-used bits - let mask = !((1 << overhang) - 1); // e.g. 5 unused bits => 111110..0 + let mask = mask_for_bits(self.nbits); if value { - self.storage[old_last_word] |= mask; + self.storage[old_last_word] |= !mask; } else { - self.storage[old_last_word] &= !mask; + // Extra bits are already supposed to be zero by invariant, but play it safe... + self.storage[old_last_word] &= mask; } }