bitvec icon indicating copy to clipboard operation
bitvec copied to clipboard

Bug?: BitVec's .as_bool() function returns negated value instead of the actual bit value

Open shrugalic opened this issue 2 years ago • 2 comments

Because I didn't know any better, when I first used BitVec (version 1.0.1) yesterday I called .as_bool() on a particular bit of a BitVec, instead of just evaluating the bit.

Surprisingly, it returned the negated bit value instead of the expected value. Here's a test to demonstrate:

#[test]
fn bitvec_bool() {
    let bits: BitVec = [true, false].into_iter().collect();
    assert!(bits[0]); // works
    assert!(!bits[1]); // works
    assert!(bits[0].as_bool()); // fails
    assert!(!bits[1].as_bool()); // fails
}

The code that causes this is part of funty 2.0.0. Unfortunately, I couldn't find an appropriate branch of funty to create a PR.

It looks like this will be fixed in funty 3.0 (its main branch seems to be the 3.0 RC1), as the code for as_bool() for bool was replaced by this:

impl Fundamental for bool {
	#[inline(always)]
	fn as_bool(self) -> bool {
		self
	}

I do want to mention that the impl_for macro reads a bit strange to me, but then again I don't have much experience with macros. It reads like this:

macro_rules! impl_for {
	(Fundamental => $($t:ty => $is_zero:expr),+ $(,)?) => { $(
		impl Fundamental for $t {
			#[inline(always)]
			#[allow(clippy::redundant_closure_call)]
			fn as_bool(self) -> bool { ($is_zero)(self) }

Here's one example usage (of many more):

impl_for!(Fundamental =>
	i8 => |this| this != 0,

So the filter predicate closure in this case (and many other cases) is |this| this != 0, and the strange (to me) part about this is that it is called $is_zero in the macro. It would make sense to me if it were called either $is_NOT_zero or is_true or bit_is_set or anything along those lines.

Or maybe I completely mis-understand the purpose of as_bool()?

shrugalic avatar Dec 31 '22 09:12 shrugalic

Hi! I apologize for the long delay; I have had a very stressful winter and have not had the time or energy to check this repository in a timely manner. I'm trying to catch up now.


Okay, so, bitslice[index] already produces a bool; you shouldn't need to change its type. But this is definitely a logic error in funty.

I'll have to fix it in funty 2, since bitvec 1 is going to keep using that. Thank you for bringing it to my attention.

myrrlyn avatar Apr 12 '23 22:04 myrrlyn

I just got bit by this, very confusing!

In my case, I was using as_bool() because I'm not familiar with BitVec and some API I'm calling returned BitVec. I wanted to iterate over the bits, so I did .iter(), and then I ended up with a BitRef, and I called as_bool() on that, causing the problem illustrated by the OP.

For completeness here's my minimal test case:

// use bitvec::prelude::*;
let interrogate_lsb = |x: BitVec<_, _>| {
    let lsb_array = x[0];
    let lsb_iter_as_bool = x.iter().next().unwrap().as_bool();
    let lsb_iter_as_u64 = x.iter().next().unwrap().as_u64();
    println!("x: {x:?}");
    println!("x[0]: {lsb_array:?}");
    println!("x.iter().next().unwrap().as_bool(): {lsb_iter_as_bool:?}");
    println!("x.iter().next().unwrap().as_u64(): {lsb_iter_as_u64:?}");
};
let mut bv = bitvec![u8, Lsb0;];
bv.push(false);
interrogate_lsb(bv);
println!();
let mut bv = bitvec![u8, Lsb0;];
bv.push(true);
interrogate_lsb(bv);

which prints

x: BitVec<u8, bitvec::order::Lsb0> { addr: 0x7fc434001f20, head: 000, bits: 1, capacity: 64 } [0]
x[0]: false
x.iter.next().unwrap(): BitRef<u8, bitvec::order::Lsb0> { addr: 0x7fc434001f20, head: 000, bits: 1, bit: false }
x.iter().next().unwrap().as_bool(): true
x.iter().next().unwrap().as_u64(): 0

x: BitVec<u8, bitvec::order::Lsb0> { addr: 0x7fc434001f20, head: 000, bits: 1, capacity: 64 } [1]
x[0]: true
x.iter.next().unwrap(): BitRef<u8, bitvec::order::Lsb0> { addr: 0x7fc434001f20, head: 000, bits: 1, bit: true }
x.iter().next().unwrap().as_bool(): false
x.iter().next().unwrap().as_u64(): 1

ntc2 avatar Dec 26 '23 11:12 ntc2