feat(clp-s::filter): Add BitmapView class for bitmaps passed across FFI boundaries.#2337
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesBitmapView Implementation, Wiring, and Tests
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/core/src/clp_s/filter/tests/test-clp_s-bitmap_view.cpp`:
- Around line 89-102: The current test only validates the error case when
set_bit is called with an out-of-bounds index. Add new TEST_CASE blocks to test
the success cases for set_bit. Create one test that calls set_bit with a valid
index to set a bit to true, then verifies the change using test_bit. Create
another test that calls set_bit with a valid index to set a bit to false, then
verifies the bit was cleared using test_bit. This will ensure the set_bit method
works correctly for both setting and clearing bits in the happy path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f79fb33-4d9e-4bd9-9ca3-8bb1160477eb
📒 Files selected for processing (4)
components/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp_s/filter/BitmapView.hppcomponents/core/src/clp_s/filter/CMakeLists.txtcomponents/core/src/clp_s/filter/tests/test-clp_s-bitmap_view.cpp
| TEST_CASE( | ||
| "BitmapView set_bit returns result_out_of_range for an out-of-bounds index", | ||
| "[clp_s][filter]" | ||
| ) { | ||
| constexpr size_t cNumBits{13}; | ||
| std::array<uint64_t, 1> storage{}; | ||
|
|
||
| auto result = clp_s::filter::BitmapView<uint64_t>::create(storage.data(), cNumBits); | ||
| REQUIRE_FALSE(result.has_error()); | ||
|
|
||
| auto const set_result = result.value().set_bit(cNumBits, true); | ||
| REQUIRE(set_result.has_error()); | ||
| REQUIRE(std::errc::result_out_of_range == set_result.error()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding explicit tests for set_bit success cases.
While the filter_set_bits tests indirectly verify bit manipulation, adding dedicated test cases that explicitly call set_bit to set a bit to true and clear a bit to false, then verify the changes with test_bit, would improve confidence in the method's correctness and make the test suite more complete.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/core/src/clp_s/filter/tests/test-clp_s-bitmap_view.cpp` around
lines 89 - 102, The current test only validates the error case when set_bit is
called with an out-of-bounds index. Add new TEST_CASE blocks to test the success
cases for set_bit. Create one test that calls set_bit with a valid index to set
a bit to true, then verifies the change using test_bit. Create another test that
calls set_bit with a valid index to set a bit to false, then verifies the bit
was cleared using test_bit. This will ensure the set_bit method works correctly
for both setting and clearing bits in the happy path.
…andidate-archive bitmap.
Replace the placeholder `clp_s::indexing::Bitmap` with `clp_s::filter::BitmapView`
as the bitmap passed to `IndexRunner::filter`. Both are non-owning, bit-packed
views designed to be passed across an FFI boundary, so the indexing interface
should build directly on the shared `BitmapView` rather than duplicating it.
- Remove `indexing/Bitmap.{hpp,cpp}` and its unit test.
- `IndexRunner::filter` now takes a `CandidateArchiveBitmapView`
(`clp_s::filter::BitmapView<uint64_t>`); an index narrows the candidate set via
`BitmapView::filter_set_bits`.
NOTE: Depends on `clp_s::filter::BitmapView` from y-scope#2337; this branch
requires that change in its base to build.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q47Ekc4mUW8XLZkouFfe6W
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
Reviewed the implementation. Haven't checked the tests yet.
| class BitmapView { | ||
| public: | ||
| // Static constants | ||
| static constexpr size_t cNumBitsPerComponent{sizeof(BitmapComponentType) * 8}; |
There was a problem hiding this comment.
I feel like this can be a private member. Is it possible that we're gonna need this outside of the member methods?
There was a problem hiding this comment.
Kind of -- I think if we add utility methods to do bitwise combinations of multiple bitmaps we might want to make that work for BitmapView classes with differently sized underlying components. May have to use friend classes for that and/or expose more public APIs for that anyway though, so I'd be fine making this private for now.
There was a problem hiding this comment.
Shall we put it in a util namespace?
There was a problem hiding this comment.
Do you mean into a utils subdirectory + namespace here, just a utils namespace here, or moving it elsewhere in the codebase as a general utility?
There was a problem hiding this comment.
I was thinking of moving it somewhere in the codebase as a general utility.
But I guess we should eventually move these things into ystdlib... so leaving it under filter namespace for now is probably ok.
| * failure: | ||
| * - std::errc::invalid_argument if either `bitmap_array` is null or `num_bits` is zero. | ||
| */ | ||
| [[nodiscard]] static auto create(BitmapComponentType* bitmap_array, size_t num_bits) |
There was a problem hiding this comment.
To be safer, I think we should use std::span instead of a raw pointer.
- Span should be the default representation for a non-owning array. It's also the way to avoid pointer arithmetic.
- When passing the array across the FFI layer, it's worth not just passing the pointer but also the length of the underlying array, along with the number of bits.
| [[nodiscard]] static auto create(BitmapComponentType* bitmap_array, size_t num_bits) | ||
| -> ystdlib::error_handling::Result<BitmapView> { | ||
| if (nullptr == bitmap_array || 0 == num_bits) { | ||
| return std::errc::invalid_argument; |
There was a problem hiding this comment.
Shall we define our own error code? I prefer us to fully move away from the std::errc since when an error happens, the top-level caller that handles the error may lose context on exactly which component raises the error.
There was a problem hiding this comment.
Going to leave this until after I'm clear on whether/where you want me to move this code.
Not sure if I agree that this disambiguates anything in a useful way for error handling/debugging anyway though -- I think it would actually be harder to work with if every container-like class defined its own error codes for common things like out of range access; I could see an argument for creating a shared error code enum in ystdlib or something that provides common error codes for container-like classes though.
There was a problem hiding this comment.
I could see an argument for creating a shared error code enum in ystdlib or something that provides common error codes for container-like classes though.
sgtm.
| if (index >= m_num_bits) { | ||
| return std::errc::result_out_of_range; | ||
| } | ||
|
|
||
| auto const component_idx{index / cNumBitsPerComponent}; | ||
| auto const bit_offset{index % cNumBitsPerComponent}; | ||
| auto const bit_mask{static_cast<BitmapComponentType>(1) << bit_offset}; |
There was a problem hiding this comment.
Nit: maybe we can have a private method to return a tuple of these three + index validation?
| * indicating the failure. | ||
| */ | ||
| template <typename SetBitVisitor> | ||
| concept SetBitVisitorConcept = requires(SetBitVisitor bit_handler, size_t bit_idx) { |
There was a problem hiding this comment.
Forgot to add this in the convention doc...
I think we should rename it SetBitVisitorReq to meet our standard.
| @@ -1,5 +1,6 @@ | |||
| set( | |||
| CLP_S_FILTER_SOURCES | |||
| BitmapView.hpp | |||
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/core/src/clp_s/filter/BitmapView.hpp`:
- Around line 122-123: The filter_set_bits template currently takes
set_bit_visitor by value, which can cause unnecessary copies for stateful or
heavy callables. Update the BitmapView::filter_set_bits signature to take
SetBitVisitor&& and forward it through the implementation, preserving the
existing Result<void> behavior while avoiding extra copies for callers that pass
movable or expensive visitor objects.
- Around line 86-109: The bitmask values are being deduced as int after integral
promotion when BitmapComponentType is narrower than int, which can trigger
narrowing/conversion issues in BitmapView. In get_component_idx_and_bitmask and
the related bit-generation logic used by test_bit, set_bit, and filter_set_bits,
explicitly cast the full shifted expression back to BitmapComponentType so
bit_mask/current_bit stay in the component type instead of relying on implicit
narrowing. Keep the existing flow and return types unchanged, just tighten the
mask type at the point it is created.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01ad2d0a-ae28-482a-b2bf-302fd298318e
📒 Files selected for processing (3)
components/core/src/clp_s/filter/BitmapView.hppcomponents/core/src/clp_s/filter/CMakeLists.txtcomponents/core/src/clp_s/filter/tests/test-clp_s-bitmap_view.cpp
| [[nodiscard]] auto filter_set_bits(SetBitVisitor set_bit_visitor) | ||
| -> ystdlib::error_handling::Result<void> { |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Visitor passed by value.
set_bit_visitor is taken by value; for stateful/heavier callables this incurs an unnecessary copy per call to filter_set_bits. Consider taking it by forwarding reference (SetBitVisitor&& set_bit_visitor) since it's already a template parameter.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/core/src/clp_s/filter/BitmapView.hpp` around lines 122 - 123, The
filter_set_bits template currently takes set_bit_visitor by value, which can
cause unnecessary copies for stateful or heavy callables. Update the
BitmapView::filter_set_bits signature to take SetBitVisitor&& and forward it
through the implementation, preserving the existing Result<void> behavior while
avoiding extra copies for callers that pass movable or expensive visitor
objects.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/core/src/clp_s/filter/BitmapView.hpp (1)
161-179: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winCast the shifted result before list-initialising
bit_mask.static_cast<BitmapComponentType>(1) << bit_offsetpromotes toint, soBitmapComponentType const bit_mask{...}narrows foruint8_t/uint16_tinstantiations and breaks this template. Usestatic_cast<BitmapComponentType>(static_cast<BitmapComponentType>(1) << bit_offset)here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/core/src/clp_s/filter/BitmapView.hpp` around lines 161 - 179, The bitmask construction in BitmapView::get_component_idx_and_bitmask is narrowing for smaller BitmapComponentType instantiations because the shift expression is promoted before list-initialization. Update the bit_mask initialization so the shifted value is cast back to BitmapComponentType before assignment, keeping the template valid for uint8_t and uint16_t. Use the get_component_idx_and_bitmask method and the bit_mask local as the fix point.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@components/core/src/clp_s/filter/BitmapView.hpp`:
- Around line 161-179: The bitmask construction in
BitmapView::get_component_idx_and_bitmask is narrowing for smaller
BitmapComponentType instantiations because the shift expression is promoted
before list-initialization. Update the bit_mask initialization so the shifted
value is cast back to BitmapComponentType before assignment, keeping the
template valid for uint8_t and uint16_t. Use the get_component_idx_and_bitmask
method and the bit_mask local as the fix point.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78b8da53-e384-4fd2-a9c3-d7a53f7bcdba
📒 Files selected for processing (1)
components/core/src/clp_s/filter/BitmapView.hpp
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
Some nit docstring fixes. Otherwise the implementation lgtm.
Will do a brief check on the test cases soon.
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
For the PR title, how about:
feat(clp-s::filter): Add `BitmapView` class for bitmaps passed across FFI boundaries.
gibber9809
changed the title
BitmapView class for bitmaps passed across FFI boundaries.
|
CI failure is unrelated to this PR -- macos build is timing out right now since it's doing an uncached clang-tidy check over the whole repo for some reason. |
2 participants
Description
As part of the extensible indexing interface, we expect to handle bitmaps passed into C++ through an FFI layer. That will generally mean that we'll be passed a bitmap as an array plus a field indicating the size of the bitmap, and have to interpret it according to some convention.
The
BitmapViewclass added in this PR is designed to work with such bitmaps, and supports arbitrary element sizes in the backing array via templates. This class implements standard bitmap methods over this backing array (test_bit/set_bit), and also offers a templated convenience method that allows users to pass a visitor function that visits and optionally modifies the state of every bit set to1in the bitmap.Checklist
breaking change.
Validation performed
BitmapView.Summary by CodeRabbit
BitmapViewwrapper for bit-level reads and writes, including visitor-based filtering of already-set bits.test_bitresults,set_bitout-of-range handling, andfilter_set_bitsbehavior across component boundaries.