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

implement Eq and Hash for some immut types #1215

Merged
merged 4 commits into from
Nov 16, 2024

Conversation

yj-qin
Copy link
Collaborator

@yj-qin yj-qin commented Nov 14, 2024

Related issue: #1177

  1. Implement Hash for @immut/array
  2. Implement Hash for @immut/list
  3. Implement Hash and Eq for @immut/priority_queue
  4. Implement Hash for @immut/sorted_map
  5. Explicit implement Eq for some types.

Copy link

peter-jerry-ye-code-review bot commented Nov 14, 2024

Observed Problems and Suggestions

  1. Redundant Implementation of op_equal in sorted_set/generic.mbt:

    • The op_equal function is implemented in sorted_set/generic.mbt, but it seems redundant given that Eq trait implementation can handle equality checks directly. This can be refactored to use the Eq trait directly.
    • Suggestion: Remove the redundant op_equal function and rely on the Eq trait implementation for equality checks.
  2. Missing Eq Trait Implementation for T[A] in sorted_set/sorted_set.mbti:

    • The op_equal function is explicitly implemented in sorted_set/generic.mbt, but there is no Eq trait implementation for T[A] in sorted_set/sorted_set.mbti.
    • Suggestion: Add the Eq trait implementation for T[A] in sorted_set/sorted_set.mbti to ensure consistency and leverage trait-based equality checks.
  3. Potential Performance Issue in op_equal Implementation:

    • The op_equal function in sorted_set/generic.mbt manually iterates through the elements to check for equality, which might be inefficient for large sets. Using the Eq trait directly can leverage optimized implementations provided by the language.
    • Suggestion: Refactor the op_equal function to use the Eq trait for better performance and code consistency.

By addressing these issues, the code will be more concise, maintainable, and potentially more efficient.

@coveralls
Copy link
Collaborator

coveralls commented Nov 14, 2024

Pull Request Test Coverage Report for Build 3871

Details

  • 5 of 5 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 80.118%

Totals Coverage Status
Change from base Build 3867: 0.06%
Covered Lines: 4356
Relevant Lines: 5437

💛 - Coveralls

///|
pub impl[A : Compare] Eq for T[A] with op_equal(self, other) {
self.length() == other.length() && self.to_array() == other.to_array()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

FTR, performance wise, this may be improved in the future

@@ -22,6 +22,8 @@ impl T {
to_string[A : Show + Compare + Eq](Self[A]) -> String
unsafe_pop[A : Compare + Eq](Self[A]) -> Self[A]
}
impl[A : Compare + Eq] Eq for T[A]
impl[A : Hash + Compare + Eq] Hash for T[A]
impl[A : Show + Compare + Eq] Show for T[A]
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @lynzrand this is redundant, given Compare: Eq

@bobzhang bobzhang merged commit f6f4836 into moonbitlang:main Nov 16, 2024
13 checks passed
@yj-qin yj-qin deleted the immut-impl branch November 16, 2024 03:39
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