Skip to content

Conversation

rami3l
Copy link
Contributor

@rami3l rami3l commented Jun 20, 2025

Note

This PR was made by an LLM agent without further modifications, and it is not intended to be merged.

Implement two new methods for the LinkedHashMap:

  1. Map::map[K, V, V2](self: Self[K, V], f: (K, V) -> V2) -> Map[K, V2]

    • Transforms all values in the map using the provided function
    • Preserves the original structure, order, and hash table layout
    • Creates a new map without rehashing
  2. Map::copy[K, V](self: Self[K, V]) -> Map[K, V]

    • Creates a deep copy of the map with identical structure and contents
    • Returned map is completely independent of the original
    • Preserves insertion order and hash table structure

Both methods avoid using Map::new() or Map::add() as required, instead directly creating maps from existing entries while preserving the internal structure.

Features:

  • Full documentation with doctests showing proper usage
  • Comprehensive black-box tests covering various scenarios
  • Preservation of insertion order and independence for copies
  • Support for type transformation in map method
  • Memory-efficient implementation using pointer comparisons

Tests included:

  • Basic functionality tests for both methods
  • Empty map handling
  • Type transformation verification
  • Independence verification (modifications to original don't affect copies)
  • Complex transformation scenarios
  • Chaining operations verification

Implement two new methods for the LinkedHashMap:

1. `Map::map[K, V, V2](self: Self[K, V], f: (K, V) -> V2) -> Map[K, V2]`
   - Transforms all values in the map using the provided function
   - Preserves the original structure, order, and hash table layout
   - Creates a new map without rehashing

2. `Map::copy[K, V](self: Self[K, V]) -> Map[K, V]`
   - Creates a deep copy of the map with identical structure and contents
   - Returned map is completely independent of the original
   - Preserves insertion order and hash table structure

Both methods avoid using `Map::new()` or `Map::add()` as required, instead directly creating maps from existing entries while preserving the internal structure.

Features:
- Full documentation with doctests showing proper usage
- Comprehensive black-box tests covering various scenarios
- Preservation of insertion order and independence for copies
- Support for type transformation in map method
- Memory-efficient implementation using pointer comparisons

Tests included:
- Basic functionality tests for both methods
- Empty map handling
- Type transformation verification
- Independence verification (modifications to original don't affect copies)
- Complex transformation scenarios
- Chaining operations verification
Copy link

Inefficient next entry lookup in map/copy methods

Category
Performance
Code Snippet
// Find which slot contains the next entry by comparing addresses
for j in 0..<self.capacity {
match self.entries[j] {
Recommendation
Store indexes instead of using physical_equal comparison:

type Entry[K, V] = {
  next_idx: Int, // Store index instead of Entry reference
  // ... other fields
}

Reasoning
The current implementation performs a linear search through all entries to find the next entry by pointer comparison. Storing direct indexes would provide O(1) access to the next entry instead of O(n) search, significantly improving performance for large maps.

Duplicated code between map and copy methods

Category
Maintainability
Code Snippet
// Second pass: rebuild linked list connections by following the original structure
let mut new_head : Entry[K, V]? = None
for i in 0..<self.capacity { ... }
Recommendation
Extract common functionality into helper methods:

fn rebuild_links[K, V](old_entries, new_entries, capacity) -> Entry[K,V]? { ... }

Reasoning
The map and copy methods share almost identical code for rebuilding the linked list structure. Extracting this into a helper method would reduce code duplication and make maintenance easier.

Tail reference is copied without rebuilding

Category
Correctness
Code Snippet
tail: self.tail,
Recommendation
Rebuild tail reference similar to head:

let mut new_tail = None
// Find tail by traversing from head
let mut current = new_head
while current.next != None {
  current = current.next
}
new_tail = current

Reasoning
The tail reference is copied directly from the source map without rebuilding the reference. This could lead to invalid tail references since the entries are new objects. The tail should be rebuilt like the head reference.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 7405

Details

  • 45 of 46 (97.83%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.04%) to 89.921%

Changes Missing Coverage Covered Lines Changed/Added Lines %
builtin/linked_hash_map.mbt 45 46 97.83%
Files with Coverage Reduction New Missed Lines %
builtin/linked_hash_map.mbt 1 95.68%
Totals Coverage Status
Change from base Build 7404: 0.04%
Covered Lines: 8547
Relevant Lines: 9505

💛 - Coveralls

@rami3l rami3l marked this pull request as draft June 20, 2025 09:06
@rami3l rami3l changed the title feat: Add Map::map and Map::copy methods to LinkedHashMap [DON'T MERGE] feat: Add Map::map and Map::copy methods to LinkedHashMap Jun 20, 2025
Comment on lines +627 to +639
// Find which slot contains the next entry by comparing addresses
for j in 0..<self.capacity {
match self.entries[j] {
Some(candidate_entry) => {
// Use pointer comparison instead of value comparison
if physical_equal(candidate_entry, old_next_entry) {
new_entry.next = new_entries[j]
break
}
}
None => continue
}
}
Copy link
Contributor

@FlyCloudC FlyCloudC Jun 21, 2025

Choose a reason for hiding this comment

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

There is no need to scan self.entries. This would lead to an O(capacity²) time complexity.

The index of old_next_entry = (old_next_entry.hash + old_next_entry.psl) % capacity.

Copy link
Contributor Author

@rami3l rami3l Jun 21, 2025

Choose a reason for hiding this comment

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

@FlyCloudC Thank you for your review! However, as the original commit message already points out, this PR is actually not written by me but an LLM agent to test out its abilities. I'll make it clearer in the original post.

@rami3l rami3l closed this Jun 21, 2025
@rami3l rami3l deleted the feat/map-methods branch June 21, 2025 04:04
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