Skip to content

fix: clone first bitmap instead of storing reference in ThreadSafeKindBitmap Or - BED-7656#41

Merged
zinic merged 1 commit intomainfrom
BED-7656
Mar 17, 2026
Merged

fix: clone first bitmap instead of storing reference in ThreadSafeKindBitmap Or - BED-7656#41
zinic merged 1 commit intomainfrom
BED-7656

Conversation

@zinic
Copy link
Copy Markdown
Contributor

@zinic zinic commented Mar 16, 2026

The ThreadSafeKindBitmp copes a reference to a given bitmap in the Or function if the kinds the bitmap is being merged into do not yet have bitmap containers in the map. This leads to strange misbehavior when the structure is used as intended as a concurrent read-write access safe structure.

Summary by CodeRabbit

  • Tests

    • Added concurrency testing to validate thread-safety of bitmap operations under concurrent access patterns.
  • Refactor

    • Optimized bitmap storage and initialization strategy.
    • Removed unused public method from bitmap class.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 16, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3c23b3a3-2f39-4ea2-9f61-a65c34a06cd4

📥 Commits

Reviewing files that changed from the base of the PR and between f97f931 and e6695f3.

📒 Files selected for processing (2)
  • graph/types.go
  • graph/types_test.go

Walkthrough

The changes optimize ThreadSafeKindBitmap by removing the public Count method and improving bitmap initialization in Add, CheckedAdd, and Or methods using direct initialization and cloning to prevent shared mutable state. A new test file validates concurrent thread-safety of these operations.

Changes

Cohort / File(s) Summary
ThreadSafeKindBitmap Optimization
graph/types.go
Removed public Count method; optimized bitmap initialization in Add and CheckedAdd using cardinality.NewBitmap64With; modified Or to clone bitmaps instead of storing references, preventing shared mutable state issues.
Concurrency Testing
graph/types_test.go
New test file with Test_ThreadSafeKindBitmap_ConcurrentAccess validating thread-safety of concurrent Or operations and Cardinality checks using goroutines, sync.WaitGroup, and context-based cancellation across 100 generated kinds.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Hopping through bitmaps with clones held tight,
No shared mutable state causing a fright!
Count removed, initialization optimized with care,
Goroutines dance safely—concurrency's fair! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: cloning a bitmap instead of storing a reference in ThreadSafeKindBitmap.Or method, with specific issue reference.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch BED-7656
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

You can generate walkthrough in a markdown collapsible section to save space.

Enable the reviews.collapse_walkthrough setting to generate walkthrough in a markdown collapsible section.

Copy link
Copy Markdown
Contributor

@urangel urangel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find! 🚀

@zinic zinic merged commit 21473e6 into main Mar 17, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants