Skip to content

Conversation

@konard
Copy link
Member

@konard konard commented Sep 14, 2025

🐛 Problem

The BitString Length property setter had several critical issues:

  1. NotImplementedException for length decrease: Setting the length to a smaller value threw NotImplementedException
  2. Confusing logic: Lines 75-83 contained unclear array clearing and bit masking logic with "What is going on here?" comments
  3. Incorrect border updates: Min/max positive word boundaries weren't updated when shrinking the bit string
  4. Edge case handling: Setting length to 0 failed, and partial word masking was unreliable

🔧 Solution

Core Implementation Changes

Replaced the problematic setter logic with:

  • Proper length expansion: Clear new words and mask off extra bits correctly
  • Length shrinking support: Clear words beyond new length and update borders
  • Correct bit masking: Handle partial words properly when changing length
  • Edge case handling: Support setting length to 0 and cross-word boundary operations
  • Border management: Update min/max positive word tracking when shrinking

Key Fixes

  1. Removed NotImplementedException - Length can now be decreased
  2. Clear confusing logic - Replaced unclear code with well-commented, logical operations
  3. Proper masking - Bits beyond the new length are properly cleared
  4. Border updates - TryShrinkBorders() is called when shrinking to maintain internal state consistency

🧪 Testing

Added comprehensive test coverage for all Length property scenarios:

  • LengthIncreaseTest - Verify length expansion preserves existing bits
  • LengthDecreaseTest - Verify length shrinking works correctly
  • LengthSetToZeroTest - Edge case of setting length to 0
  • LengthCrossWordBoundaryTest - Operations that cross 64-bit word boundaries
  • LengthPartialWordMaskingTest - Verify proper bit masking within words
  • LengthSameValueTest - No-op when setting to same value

All existing tests continue to pass (26/26 tests passing).

📝 Example Usage

var bitString = new BitString(64);
bitString[5] = true;
bitString[45] = true;

// This now works (previously threw NotImplementedException)
bitString.Length = 32;
Console.WriteLine(bitString[5]);  // True (preserved)
// bitString[45] is no longer accessible

// Setting to 0 also works
bitString.Length = 0;

🔗 Fixes

Fixes #21


🤖 Generated with Claude Code

Adding CLAUDE.md with task information for AI processing.
This file will be removed when the task is complete.

Issue: #21
@konard konard self-assigned this Sep 14, 2025
konard and others added 2 commits September 14, 2025 13:55
- Remove NotImplementedException for length decrease operations
- Implement proper shrinking logic with correct bit masking
- Fix array clearing and word boundary handling logic
- Add comprehensive tests for all Length property scenarios
- Ensure min/max positive word boundaries are updated correctly
- Handle edge cases like setting length to 0 and partial word masking

This resolves all issues identified in the original Length property setter
including the confusing logic around lines 75-83 in the original code.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Remove NotImplementedException for length decrease operations
- Implement proper shrinking logic with correct bit masking
- Fix confusing array clearing and word boundary handling logic
- Handle edge cases like setting length to 0 and partial word masking
- Ensure min/max positive word boundaries are updated correctly when shrinking

This addresses the core issues identified in issue #21, specifically the
problematic logic around lines 75-83 in the original implementation.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@konard konard changed the title [WIP] Rethink and test Length property implementation Fix BitString Length property implementation Sep 14, 2025
@konard konard marked this pull request as ready for review September 14, 2025 10:55
🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>'
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.

Rethink and test Length property implementation

2 participants