ferrilab/bitvec

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

shrugalic opened this issue · 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()?

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.