feat: add bitmap utility functions #11
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds new bitmap utility functions to the buffers module and corresponding tests, alongside introducing helper functions in the fixtures for boolean arrays.
- Added new bitmap functions (e.g., bit_count, count_leading_bits, partial_byte_set, unsafe_range_set) in the buffers module.
- Updated tests in test_buffers.mojo to verify bitmap behavior.
- Created a new arrays.mojo file with helper functions (including assert_bitmap_set) to support tests.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| firebolt/tests/test_buffers.mojo | Added tests to validate new bitmap functions. |
| firebolt/test_fixtures/arrays.mojo | New helper functions for boolean arrays and bitmap assertions. |
| firebolt/buffers.mojo | Introduced bitmap utilities and related bit-counting functions. |
firebolt/buffers.mojo
Outdated
| @@ -82,6 +88,23 @@ struct Buffer(Movable): | |||
| else: | |||
| return self.size // sizeof[T]() | |||
|
|
|||
| fn bit_count(self) -> Int: | |||
There was a problem hiding this comment.
Shouldn't this be defined on the Bitmap struct?
firebolt/tests/test_buffers.mojo
Outdated
| def test_count_leading_bits(): | ||
| var b = Bitmap.alloc(10) | ||
| var expected_bits = b.length() | ||
| assert_equal(b.count_leading_bits(), expected_bits) |
There was a problem hiding this comment.
count_leading_bits() is a little bit confusing, I would expect to count the set bits by default. Maybe we should rename it to count_leading_zeros()/count_leading_ones()?
There was a problem hiding this comment.
Agree, added nicer APIs on top of the existing implementation.
kszucs
changed the title
| ) | ||
|
|
||
|
|
||
| @always_inline |
There was a problem hiding this comment.
I assume it doesn't need to be inlines since it is a testing utility.
There was a problem hiding this comment.
This is needed to get a nice error message for failed tests. Removing the @always_inline decoration leads to
open-source/max/mojo/stdlib/stdlib/builtin/_location.mojo:111:6: error: call location was not inlined the specified number of times: requires 1 more time(s)
Included from /var/folders/yg/_vw4ktqx5dn6t75j_hng0tvc0000gn/T/test-ca6050.mojo:8:
Included from /Users/mseritan/dev/typed-data/firebolt/bitmap-utility-functions/firebolt/tests/test_buffers.mojo:3:
/Users/mseritan/dev/typed-data/firebolt/bitmap-utility-functions/./firebolt/test_fixtures/arrays.mojo:66:37: note: called from
location=__call_location(),
firebolt/tests/test_buffers.mojo
Outdated
| assert_equal(b.count_leading_bits(1), expected_bits - 1) | ||
| b.unsafe_set(0, False) | ||
|
|
||
| var to_test = List(0, 1, 7, 8, 10, 16, 31) |
There was a problem hiding this comment.
Seems like this list is not being used just its length.
There was a problem hiding this comment.
Good catch, that was a bug in the test.
firebolt/tests/test_buffers.mojo
Outdated
| bitmap.unsafe_range_set(0, 0, True) | ||
| assert_bitmap_set(bitmap, List[Int](), "range 0") | ||
|
|
||
| var to_test = List(0, 1, 7, 8, 15) |
There was a problem hiding this comment.
Out of curiosity, is it possible to use list literals in mojo nowadays?
There was a problem hiding this comment.
Just tried it and it seems that Chris Lattner fixed that.
|
Also there are bit utilities in the stdlib which we could use here https://docs.modular.com/mojo/stdlib/bit/bit/ |
winding-lines
force-pushed
the
feat/bitmap-utility-functions
branch
from
f2ea541 to
984c764
Compare
They don't take a start position, I based this code on that implementation. |
|
@kszucs ready for re-review 🤗 |
Add some bitmap utility functions and related tests.