8000 change return type of `@sorted_map.{keys, values}` to `Iter` by FlyCloudC · Pull Request #2123 · moonbitlang/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

change return type of @sorted_map.{keys, values} to Iter #2123

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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

change return type of @sorted_map.{keys, values} to Iter #2123

wants to merge 2 commits into from

Conversation

FlyCloudC
Copy link
Contributor

see #2108

@peter-jerry-ye-code-review peter-jerry-ye code review
Copy link
peter-jerry-ye-code-review bot commented May 18, 2025 •
edited
Loading
New iterator-based implementation is more memory efficient

Category
Performance
Code Snippet
pub fn[K, V] keys_as_iter(self : T[K, V]) -> Iter[K] {
Iter::new(fn(yield_) {...})
Recommendation
The change is good - continuing with replacing other Array-returning methods with iterator versions where possible would be beneficial
Reasoning
Returning iterators instead of arrays avoids unnecessary memory allocation and copying when the full array isn't needed. This is especially important for large collections where only partial traversal may be required.

Consistent deprecation notices

Category
Maintainability
Code Snippet
#deprecated("Use keys_as_iter instead. keys will return Iter[K] instead of Array[K] in the future.")
Recommendation
Consider adding a timeline for when the deprecated methods will be removed
Reasoning
The deprecation notices are clear about the replacement methods, but adding a timeline helps users plan their migration. For example: 'Will be removed in version X.Y.Z'

Tree traversal order is preserved in new iterator implementation

Category
Correctness
Code Snippet
fn go(x : Node[K, V]?) {
match x {
None => IterContinue
Some({ left, right, key, .. }) =>
Recommendation
Current implementation correctly maintains in-order traversal which is essential for sorted map. No changes needed.
Reasoning
The iterator implementation preserves the binary search tree's in-order traversal ensuring keys are yielded in sorted order, which is a critical invariant for a sorted map.

@coveralls
Copy link
Collaborator
coveralls commented May 18, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 7421

Details

  • 17 of 21 (80.95%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 90.072%
Changes Missing Coverage Covered Lines Changed/Added Lines %
sorted_map/map.mbt 16 20 80.0%
Totals Coverage Status
Change from base Build 7419: -0.03%
Covered Lines: 8537
Relevant Lines: 9478
💛 - Coveralls

@FlyCloudC FlyCloudC force-pushed the xxmap-keys-values branch from cdbb06e to 5c174ea Compare May 18, 2025 14:40
@FlyCloudC FlyCloudC marked this pull request as ready for review May 18, 2025 15:03
@FlyCloudC FlyCloudC mentioned this pull request May 18, 2025
Merged
@peter-jerry-ye
Copy link
Collaborator

Same as #2124, this introduces breaking changes. It would be better to have some migration process for users.

@FlyCloudC
Copy link
Contributor Author

Thanks for the feedback. I’d like to ensure we handle the migration correctly. Since this is a return type change (not just a rename), could you clarify the preferred way to deprecate the old behavior while maintaining backward compatibility?

I noticed PR #1861 made a similar change directly—should we follow that precedent, or is there a different pattern you’d recommend for this case?

@FlyCloudC FlyCloudC closed this May 19, 2025
@FlyCloudC FlyCloudC force-pushed the xxmap-keys-values branch from 5c174ea to c2f9399 Compare May 19, 2025 09:46
@FlyCloudC FlyCloudC reopened this May 19, 2025
@FlyCloudC FlyCloudC force-pushed the xxmap-keys-values branch from b3df260 to 389ff77 Compare May 19, 2025 15:28
@peter-jerry-ye
Copy link
Collaborator

Yeah, I admit that #1861 did it abruptly. If possible, I would like you to define a new method, probably something like keys_as_iter, and add deprecation to the keys and values to let people know about that this is actually happening.

@FlyCloudC FlyCloudC marked this pull request as draft May 20, 2025 12:44
@FlyCloudC
Copy link
Contributor Author

Waiting for #2124 to be merged first.

@FlyCloudC FlyCloudC changed the title change return type of @sorted_map.keys/values to Iter change return type of @sorted_map.{keys, values} to Iter May 24, 2025
@FlyCloudC FlyCloudC force-pushed the xxmap-keys-values branch from 754cd80 to 00e668a Compare June 21, 2025 14:42
@FlyCloudC FlyCloudC marked this pull request as ready for review June 21, 2025 14:42
@peter-jerry-ye peter-jerry-ye requested a review from bobzhang June 23, 2025 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bobzhang bobzhang Awaiting requested review from bobzhang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FlyCloudC @coveralls @peter-jerry-ye
0