-
Notifications
You must be signed in to change notification settings - Fork 111
[utils] Implement rewindable bitmap #1717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a rewindable bitmap wrapper called Historical
that maintains a current state plus diffs for tracking history and batching operations. The implementation provides diff-based snapshots, atomic batch operations with read-through semantics, and the ability to reconstruct any historical state.
Key changes:
- Adds a new
Historical
bitmap wrapper with batch-based mutation system - Implements reverse diff tracking to reconstruct historical states
- Provides atomic batch operations with automatic abort on drop
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
utils/src/bitmap/mod.rs | Exports the new Historical , BatchGuard , and Error types |
utils/src/bitmap/historical.rs | Complete implementation of the rewindable bitmap with extensive documentation and tests |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
let mut historical: Historical<4> = Historical::new(); | ||
let _batch1 = historical.start_batch(); | ||
// This should panic because a batch is already active | ||
// We need to use core::mem::forget to prevent drop from clearing the batch | ||
core::mem::forget(_batch1); |
Copilot
AI
Oct 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using core::mem::forget
in tests can lead to resource leaks and makes the test harder to understand. Consider restructuring the test to avoid needing mem::forget
, perhaps by extracting the panic-inducing code into a separate function.
let mut historical: Historical<4> = Historical::new(); | |
let _batch1 = historical.start_batch(); | |
// This should panic because a batch is already active | |
// We need to use core::mem::forget to prevent drop from clearing the batch | |
core::mem::forget(_batch1); | |
start_second_batch_panics(); | |
} | |
fn start_second_batch_panics() { | |
let mut historical: Historical<4> = Historical::new(); | |
let _batch1 = historical.start_batch(); | |
// This should panic because a batch is already active |
Copilot uses AI. Check for mistakes.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1717 +/- ##
==========================================
+ Coverage 92.10% 92.15% +0.04%
==========================================
Files 307 308 +1
Lines 79648 80794 +1146
==========================================
+ Hits 73362 74455 +1093
- Misses 6286 6339 +53
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Work in progress