8000 feat(Map): add `map` and `copy` for `Map` by peter-jerry-ye · Pull Request #2228 · moonbitlang/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(Map): add map and copy for Map #2228

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 22, 2025
Merged

feat(Map): add map and copy for Map #2228

merged 3 commits into from
Jun 22, 2025

Conversation

peter-jerry-ye
Copy link
Collaborator

Replaces #2210
Closes #1890

@peter-jerry-ye-code-review peter-jerry-ye code review
Copy link
peter-jerry-ye-code-review bot commented Jun 9, 2025 •
edited
Loading
Significant code duplication between Map::map and Map::copy implementations

Category
Maintainability
Code Snippet
// Both methods contain nearly identical structure copying and iteration logic
let other = {
capacity: self.capacity,
entries: FixedArray::make(self.capacity, None),
...
}
Recommendation
Extract the common copy logic into a private helper function like _copy_structure that takes a transformation function as parameter. Then implement both methods using this helper.
Reasoning
The current implementation duplicates complex logic for structure copying and linked list traversal. This increases maintenance burden and risk of bugs when modifying either method.

Missing validation for capacity initialization

Category
Correctness
Code Snippet
entries: FixedArray::make(self.capacity, None)
Recommendation
Add validation to ensure self.capacity is positive before creating the FixedArray
Reasoning
While unlikely due to Map's implementation, creating a FixedArray with negative or zero capacity could cause runtime errors. Defensive programming would add a safety check.

Test cases could be more comprehensive

Category
Maintainability
Code Snippet
test "Map::map" {
let map = { "a": 1, "b": 2, "c": 3 }
...
}
Recommendation
Add test cases for error conditions and edge cases: very large maps, maps with complex value types, error handling in the mapping function
Reasoning
Current tests only cover basic happy path scenarios. More extensive testing would help ensure robustness and catch corner cases.

@coveralls
Copy link
Collaborator
coveralls commented Jun 9, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 7427

Details

  • 15 of 15 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 90.115%
Totals Coverage Status
Change from base Build 7425: 0.02%
Covered Lines: 8542
Relevant Lines: 9479
💛 - Coveralls

@peter-jerry-ye peter-jerry-ye mentioned this pull request Jun 9, 2025
Closed
@peter-jerry-ye
Copy link
Collaborator Author
peter-jerry-ye commented Jun 13, 2025 •
edited
Loading

We will add copy: (Self, f?:(K, V) -> V). The benefit is that we can have a fast path when there's no mapping function, and it can perform all three functionalities.

We will merge map_with_key and map to have (Self, f: (K, V) -> V2) to simplify the APIs.

@peter-jerry-ye peter-jerry-ye force-pushed the Asterless/map-enhancement-1 branch from a1ceb63 to 72fe11d Compare June 20, 2025 03:07
@peter-jerry-ye peter-jerry-ye changed the title feat(Map): add map and map_with_key for Map feat(Map): add map and copy for Map Jun 20, 2025
@peter-jerry-ye peter-jerry-ye force-pushed the Asterless/map-enhancement-1 branch from 5576b2f to f3be4ff Compare June 20, 2025 10:12
@FlyCloudC
Copy link
Contributor

Could we modify the trait bounds from fn[K: Hash + Eq, V] to fn[K, V]?

@bobzhang bobzhang force-pushed the Asterless/map-enhancement-1 branch from f3be4ff to cad7b01 8000 Compare June 22, 2025 08:11
@bobzhang bobzhang enabled auto-merge (rebase) June 22, 2025 08:14
@bobzhang bobzhang merged commit 13f1015 into main Jun 22, 2025
12 checks passed
@bobzhang bobzhang deleted the Asterless/map-enhancement-1 branch June 22, 2025 08:22
@bobzhang
Copy link
Contributor

Could we modify the trait bounds from fn[K: Hash + Eq, V] to fn[K, V]?

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] add Map::map and Map::mapi
4 participants
@peter-jerry-ye @coveralls @FlyCloudC @bobzhang
0