Fixed panic in bitmap assign_mut

[ritchie46]

The length taken into account to compute the remainder bytes could differ from the length of the buffer (e.g. in slicing operations).

This ensures we do the computations on the length of the buffer and we've added a test to ensure we don't regress.

When calling into_mut on a Bitmap and the data has an offset we memcpy the bytes into a MutableBitmap. However this is incorrect, because a MutableBitmap does not have an offset.

This fixes that by iterating every bit and recreating the proper bytes for the MutableBitmap.

This is expensive, so maybe we should consider adding an offset on MutableBitmap as well.

[codecov]

Codecov Report

Merging #1078 (e577427) into main (39db6fb) will increase coverage by 0.00%.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##             main    #1078   +/-   ##
=======================================
  Coverage   81.01%   81.01%           
=======================================
  Files         366      366           
  Lines       35056    35061    +5     
=======================================
+ Hits        28399    28404    +5     
  Misses       6657     6657           

Impacted Files	Coverage Δ
src/bitmap/immutable.rs	82.69% <66.66%> (+0.87%)	⬆️
src/array/primitive/mod.rs	84.46% <0.00%> (-1.25%)	⬇️
src/bitmap/assign_ops.rs	85.48% <0.00%> (+1.61%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39db6fb...e577427. Read the comment docs.

[jorgecarleitao]

I think we can improve this further by using iter_chunk, which will iterate in chunks of u64. We have a function somewhere to create a MutableBitmap from chunks.

[ritchie46]

I tried this, but that seems to not include the remainder

                 let chunks = BitChunks::<u64>::new(data.bytes.as_ref(), data.offset, data.length);
                    chunk_iter_to_vec(chunks)

[jorgecarleitao]

Just to double check, before the test would panic (not trigger UB), right?

[ritchie46]

Just to double check, before the test would panic (not trigger UB), right?

Yes

[jorgecarleitao]

Thanks a lot! 🙇