Skip to content
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.

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

improve each for sorted_set and refactor eachi #1458

Merged

Conversation

illusory0x0
Copy link
Contributor

Copy link

‼️ This code review is generated by a bot. Please verify the content before trusting it.

Here are three observations from the provided git diff output:

  1. Removed Iterative Traversal Implementation:

    • The original implementation of the each function for Node[V] used an iterative approach with a stack (s) to traverse the tree. This implementation has been removed entirely. While the new implementation using recursion (dfs) is simpler, it might lead to stack overflow issues for very large trees due to deep recursion. If the tree depth is significant, the iterative approach might have been more robust.
  2. Inconsistent Functionality in eachi:

    • The eachi function was originally implemented using a recursive dfs function, which directly incremented the index i during traversal. The new implementation reuses the each function and increments the index within a closure. While this is a valid refactor, it introduces a dependency on the each function. If the each function is modified in the future, it could inadvertently affect the behavior of eachi. This coupling might make the code harder to maintain.
  3. Potential Performance Impact:

    • The new eachi implementation introduces an additional closure (fn(v) { ... }) for each element in the set. While this is syntactically cleaner, it might have a slight performance overhead compared to the original direct traversal approach. If performance is critical, this change should be benchmarked to ensure it doesn't introduce a bottleneck.

These observations highlight potential trade-offs in readability, maintainability, and performance. Depending on the context, these changes might be acceptable or might require further consideration.

@coveralls
Copy link
Collaborator

coveralls commented Jan 10, 2025

Pull Request Test Coverage Report for Build 4698

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 82.973%

Totals Coverage Status
Change from base Build 4690: -0.02%
Covered Lines: 4756
Relevant Lines: 5732

💛 - Coveralls

@illusory0x0 illusory0x0 reopened this Jan 10, 2025
@bobzhang bobzhang merged commit 3892504 into moonbitlang:main Jan 11, 2025
28 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.

4 participants