Skip to content

Correct bitv build failures and a bug in all() #1

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

Merged
merged 3 commits into from
Dec 10, 2014
Merged

Correct bitv build failures and a bug in all() #1

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ab734ca
bitv: correct build failures
cuviper Dec 10, 2014
2a6e5b1
bitv: Fix all() for nbits that are multiples of u32::BITS
cuviper Dec 10, 2014
0f70e1a
bitv: reuse mask_for_bits() in grow()
cuviper Dec 10, 2014
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
66 changes: 51 additions & 15 deletions src/libcollections/bit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ use std::hash;

use vec::Vec;

type Blocks<'a> = Cloned<Items<'a, u32>>
type MutBlocks<'a> MutItems<'a, u32>;
type Blocks<'a> = Cloned<Items<'a, u32>>;
type MutBlocks<'a> = MutItems<'a, u32>;
type MatchWords<'a> = Chain<Enumerate<Blocks<'a>>, Skip<Take<Enumerate<Repeat<u32>>>>>;

// Take two BitV's, and return iterators of their words, where the shorter one
Expand Down Expand Up @@ -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)
Copy link
Owner

Choose a reason for hiding this comment

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

Niiice

Copy link
Author

Choose a reason for hiding this comment

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

It just struck me that this might be undefined behavior when the mod is zero -- that a full right shift keeps all bits. It does pass the test, at least on x86_64, but it might need an additional % u32::BITS on the shift amount to be safe. (Optimized code will turn that into a bitand, if anything.) What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

Oops, yes. This is invoking Undefined Behaviour in LLVM right now :(

Copy link
Owner

Choose a reason for hiding this comment

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

This instruction always performs a logical shift right operation. The most significant bits of the result will be filled with zero bits after the shift. If op2 is (statically or dynamically) equal to or larger than the number of bits in op1, the result is undefined.

docs
outstanding issue

Copy link
Author

Choose a reason for hiding this comment

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

OK, I'll mask it then, and send a new PR.

}

impl Bitv {
/// Applies the given operation to the blocks of self and other, and sets self to
/// be the result.
Expand All @@ -199,7 +205,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()
Copy link
Owner

Choose a reason for hiding this comment

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

I believe this can be [mut ..blocks]

Copy link
Owner

Choose a reason for hiding this comment

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

But this is basically fine

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I wasn't aware of that syntax, thanks!

}

/// Iterator over the underlying blocks of data
Expand Down Expand Up @@ -336,7 +342,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
)
}
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -788,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;
}
}

Expand Down Expand Up @@ -835,10 +841,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)
}
}
Expand Down Expand Up @@ -1794,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]
Expand All @@ -1810,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]
Expand All @@ -1820,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);
Expand All @@ -1833,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);
Expand All @@ -1842,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);
Expand All @@ -1850,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]
Expand All @@ -1862,13 +1878,15 @@ 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);
assert!(act.eq_vec(
&[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);
Expand All @@ -1884,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);
Expand All @@ -1899,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);
Expand All @@ -1913,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);
Expand All @@ -1923,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]
Expand All @@ -1935,13 +1957,15 @@ 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);
assert!(act.eq_vec(
&[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);
Expand All @@ -1957,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);
Expand All @@ -1972,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);
Expand All @@ -1987,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);
Expand All @@ -1998,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]
Expand All @@ -2010,13 +2038,15 @@ 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);
assert!(act.eq_vec(
&[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);
Expand All @@ -2032,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);
Expand All @@ -2047,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);
Expand All @@ -2062,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);
Expand All @@ -2074,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]
Expand Down Expand Up @@ -2191,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]
Expand Down