Skip to content

Conversation

danlaine
Copy link
Collaborator

Work in progress

@danlaine danlaine moved this from Staged to In Progress in Tracker Oct 2, 2025
@danlaine danlaine requested a review from Copilot October 2, 2025 17:47
Copy link
Contributor

@Copilot Copilot AI left a 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.

Comment on lines +1297 to +1301
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);
Copy link

Copilot AI Oct 2, 2025

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.

Suggested change
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.

Base automatically changed from danlaine/separate-bitmap to main October 9, 2025 20:25
Copy link

codecov bot commented Oct 13, 2025

Codecov Report

❌ Patch coverage is 95.37522% with 53 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.15%. Comparing base (9c7abdc) to head (a009707).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
utils/src/bitmap/historical.rs 95.71% 48 Missing ⚠️
utils/src/bitmap/prunable.rs 82.35% 3 Missing ⚠️
utils/src/bitmap/mod.rs 80.00% 2 Missing ⚠️
@@            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     
Files with missing lines Coverage Δ
utils/src/bitmap/mod.rs 97.04% <80.00%> (-0.15%) ⬇️
utils/src/bitmap/prunable.rs 98.22% <82.35%> (-0.50%) ⬇️
utils/src/bitmap/historical.rs 95.71% <95.71%> (ø)

Continue to review full report in Codecov by Sentry.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

1 participant