BitVec: Improve the encoding and consolidate the implementations (#327)

* BitVec: Improve the encoding and consolidate the implementations

* Bump version

* Remove debugging stuff 🙈

* Feature gate the test

* More testing

* More tests

Cargo.toml

@@ -1,7 +1,7 @@
 [package]
 name = "parity-scale-codec"
 description = "SCALE - Simple Concatenating Aggregated Little Endians"
-version = "3.1.0"
+version = "3.1.1"
 authors = ["Parity Technologies <admin@parity.io>"]
 license = "Apache-2.0"
 repository = "https://github.com/paritytech/parity-scale-codec"

src/bit_vec.rs

@@ -15,17 +15,13 @@
 //! `BitVec` specific serialization.
 
 use bitvec::{
-	vec::BitVec, store::BitStore, order::BitOrder, slice::BitSlice, boxed::BitBox,
+	vec::BitVec, store::BitStore, order::BitOrder, slice::BitSlice, boxed::BitBox, view::BitView,
 };
 use crate::{
-	EncodeLike, Encode, Decode, Input, Output, Error, Compact,
-	codec::{decode_vec_with_len, encode_slice_no_len},
+	EncodeLike, Encode, Decode, Input, Output, Error, Compact, codec::decode_vec_with_len,
 };
 
-impl<O: BitOrder, T: BitStore> Encode for BitSlice<T, O>
-	where
-		T::Mem: Encode
-{
+impl<O: BitOrder, T: BitStore + Encode> Encode for BitSlice<T, O> {
 	fn encode_to<W: Output + ?Sized>(&self, dest: &mut W) {
 		let bits = self.len();
 		assert!(
@@ -34,23 +30,18 @@ impl<O: BitOrder, T: BitStore> Encode for BitSlice<T, O>
 		);
 		Compact(bits as u32).encode_to(dest);
 
-		for element in self.domain() {
+		// Iterate over chunks
+		for chunk in self.chunks(core::mem::size_of::<T>() * 8) {
+			let mut element = T::ZERO;
+			element.view_bits_mut::<O>()[..chunk.len()].copy_from_bitslice(chunk);
 			element.encode_to(dest);
 		}
 	}
 }
 
 impl<O: BitOrder, T: BitStore + Encode> Encode for BitVec<T, O> {
 	fn encode_to<W: Output + ?Sized>(&self, dest: &mut W) {
-		let bits = self.len();
-		assert!(
-			bits <= ARCH32BIT_BITSLICE_MAX_BITS,
-			"Attempted to encode a BitVec with too many bits.",
-		);
-		Compact(bits as u32).encode_to(dest);
-
-		let slice = self.as_raw_slice();
-		encode_slice_no_len(slice, dest)
+		self.as_bitslice().encode_to(dest)
 	}
 }
 
@@ -84,15 +75,7 @@ impl<O: BitOrder, T: BitStore + Decode> Decode for BitVec<T, O> {
 
 impl<O: BitOrder, T: BitStore + Encode> Encode for BitBox<T, O> {
 	fn encode_to<W: Output + ?Sized>(&self, dest: &mut W) {
-		let bits = self.len();
-		assert!(
-			bits <= ARCH32BIT_BITSLICE_MAX_BITS,
-			"Attempted to encode a BitBox with too many bits.",
-		);
-		Compact(bits as u32).encode_to(dest);
-
-		let slice = self.as_raw_slice();
-		encode_slice_no_len(slice, dest)
+		self.as_bitslice().encode_to(dest)
 	}
 }
 
@@ -107,8 +90,8 @@ impl<O: BitOrder, T: BitStore + Decode> Decode for BitBox<T, O> {
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use bitvec::{bitvec, order::Msb0};
-	use crate::codec::MAX_PREALLOCATION;
+	use bitvec::{bitvec, order::{Msb0, Lsb0}};
+	use crate::{codec::MAX_PREALLOCATION, CompactLen};
 
 	macro_rules! test_data {
 		($inner_type:ident) => (
@@ -150,8 +133,9 @@ mod tests {
 			let encoded = v.encode();
 			assert_eq!(*v, BitVec::<u8, Msb0>::decode(&mut &encoded[..]).unwrap());
 
-			let encoded = v.as_bitslice().encode();
-			assert_eq!(*v, BitVec::<u8, Msb0>::decode(&mut &encoded[..]).unwrap());
+			let elements = bitvec::mem::elts::<u8>(v.len());
+			let compact_len = Compact::compact_len(&(v.len() as u32));
+			assert_eq!(compact_len + elements, encoded.len(), "{}", v);
 		}
 	}
 
@@ -161,8 +145,9 @@ mod tests {
 			let encoded = v.encode();
 			assert_eq!(*v, BitVec::<u16, Msb0>::decode(&mut &encoded[..]).unwrap());
 
-			let encoded = v.as_bitslice().encode();
-			assert_eq!(*v, BitVec::<u16, Msb0>::decode(&mut &encoded[..]).unwrap());
+			let elements = bitvec::mem::elts::<u16>(v.len());
+			let compact_len = Compact::compact_len(&(v.len() as u32));
+			assert_eq!(compact_len + elements * 2, encoded.len(), "{}", v);
 		}
 	}
 
@@ -172,8 +157,9 @@ mod tests {
 			let encoded = v.encode();
 			assert_eq!(*v, BitVec::<u32, Msb0>::decode(&mut &encoded[..]).unwrap());
 
-			let encoded = v.as_bitslice().encode();
-			assert_eq!(*v, BitVec::<u32, Msb0>::decode(&mut &encoded[..]).unwrap());
+			let elements = bitvec::mem::elts::<u32>(v.len());
+			let compact_len = Compact::compact_len(&(v.len() as u32));
+			assert_eq!(compact_len + elements * 4, encoded.len(), "{}", v);
 		}
 	}
 
@@ -182,6 +168,10 @@ mod tests {
 		for v in &test_data!(u64) {
 			let encoded = v.encode();
 			assert_eq!(*v, BitVec::<u64, Msb0>::decode(&mut &encoded[..]).unwrap());
+
+			let elements = bitvec::mem::elts::<u64>(v.len());
+			let compact_len = Compact::compact_len(&(v.len() as u32));
+			assert_eq!(compact_len + elements * 8, encoded.len(), "{}", v);
 		}
 	}
 
@@ -203,4 +193,19 @@ mod tests {
 		let decoded = BitBox::<u8, Msb0>::decode(&mut &encoded[..]).unwrap();
 		assert_eq!(bb, decoded);
 	}
+
+	#[test]
+	fn bitvec_u8_encodes_as_expected() {
+		let cases = vec![
+			(bitvec![u8, Lsb0; 0, 0, 1, 1].encode(), (Compact(4u32), 0b00001100u8).encode()),
+			(bitvec![u8, Lsb0; 0, 1, 1, 1].encode(), (Compact(4u32), 0b00001110u8).encode()),
+			(bitvec![u8, Lsb0; 1, 1, 1, 1].encode(), (Compact(4u32), 0b00001111u8).encode()),
+			(bitvec![u8, Lsb0; 1, 1, 1, 1, 1].encode(), (Compact(5u32), 0b00011111u8).encode()),
+			(bitvec![u8, Lsb0; 1, 1, 1, 1, 1, 0].encode(), (Compact(6u32), 0b00011111u8).encode()),
+		];
+
+		for (idx, (actual, expected)) in cases.into_iter().enumerate() {
+			assert_eq!(actual, expected, "case at index {} failed; encodings differ", idx);
+		}
+	}
 }

tests/mod.rs

@@ -15,7 +15,7 @@
 #[cfg(not(feature="derive"))]
 use parity_scale_codec_derive::{Encode, Decode};
 use parity_scale_codec::{
-	Encode, Decode, HasCompact, Compact, EncodeAsRef, CompactAs, Error, Output,
+	Encode, Decode, HasCompact, Compact, EncodeAsRef, CompactAs, Error, Output, DecodeAll
 };
 use serde_derive::{Serialize, Deserialize};
 
@@ -586,3 +586,35 @@ fn custom_trait_bound() {
 
 	Something::<NotEncode, u32>::decode(&mut &encoded[..]).unwrap();
 }
+
+#[test]
+#[cfg(feature = "bit-vec")]
+fn bit_vec_works() {
+	use bitvec::prelude::*;
+
+	// Try some fancy stuff
+	let original_vec = bitvec![u8, Msb0; 1; 8];
+	let mut original_vec_clone = original_vec.clone();
+	original_vec_clone = original_vec_clone.split_off(5);
+	original_vec_clone.push(true);
+	original_vec_clone.push(true);
+	original_vec_clone.push(true);
+	original_vec_clone.push(true);
+	original_vec_clone.push(true);
+
+	assert_eq!(original_vec, original_vec_clone);
+
+	#[derive(Decode, Encode, PartialEq, Debug)]
+	struct MyStruct {
+		v: BitVec<u8, Msb0>,
+		x: u8,
+	}
+
+	let v1 = MyStruct { v: original_vec, x: 42 }.encode();
+	let v2 = MyStruct { v: original_vec_clone, x: 42 }.encode();
+	assert_eq!(v1, v2);
+
+	let v1 = MyStruct::decode(&mut &v1[..]).unwrap();
+	let v2 = MyStruct::decode_all(&mut &v2[..]).unwrap();
+	assert_eq!(v1.x, v2.x);
+}