add compare for sorted_set

[illusory0x0]

No description provided.

[peter-jerry-ye-code-review]

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

Here are three observations and potential issues from the provided git diff:

1. Potential Issue in compare Implementation:

   • The compare function in set.mbt uses a depth-first search (DFS) approach to compare elements of two sets. However, the DFS traversal might not correctly handle cases where the sets have the same elements but in different orders due to the tree structure. This could lead to incorrect comparison results. For example, if the tree structure is unbalanced, the traversal order might not match the sorted order of the elements, leading to incorrect comparisons.

2. Redundant Comparison Logic:

   • The compare function includes a fallback comparison based on the sizes of the sets (self.size - other.size). While this is a valid approach, it might be redundant if the DFS traversal already correctly identifies differences in the elements. This redundancy could lead to confusion or inefficiency in the code.

3. Test Case Coverage:

   • The new test cases in set_test.mbt for the compare function only test basic scenarios (e.g., comparing sets of different sizes). There are no test cases for more complex scenarios, such as comparing sets with the same elements but different tree structures, or comparing sets with elements that are not in the same order. This lack of comprehensive test coverage could miss edge cases where the compare function might fail.

These observations suggest that the compare function might need further refinement and additional test cases to ensure it behaves correctly in all scenarios.

[coveralls]

Pull Request Test Coverage Report for Build 4741

Details

• 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.02%) to 82.867%

---

Totals
Change from base Build 4739:	0.02%
Covered Lines:	4827
Relevant Lines:	5825

---

💛 - Coveralls

[bobzhang]

cc @hackwaly I remember you wrote one reasonably efficient version before?

[hackwaly]

It's there:

core/immut/sorted_set/generic.mbt

Lines 64 to 77 in b45fcd7

	pub impl[A : Compare] Compare for T[A] with compare(self, other) -> Int {
	let iter = InorderIterator::new(self)
	let iter1 = InorderIterator::new(other)
	loop iter.next(), iter1.next() {
	None, None => 0
	Some(a), Some(b) => {
	let cmp = a.compare(b)
	guard cmp == 0 else { break cmp }
	continue iter.next(), iter1.next()
	}
	Some(_), None => 1
	None, Some(_) => -1
	}
	}

[peter-jerry-ye]

I think the design principle for comparing containers in core is to compare the size first?

[Lampese]

I have one concern: Will structure-based comparisons affect the results of the comparison because of the order in which the nodes are inserted? I'll test that out later.

[hackwaly]

I think the design principle for comparing containers in core is to compare the size first?

I don't think so. Usually, String::compare isn't, and Some containers don't have O(1) length at all.

but, here we can compare size first due to performance reason.

[peter-jerry-ye]

I think the design principle for comparing containers in core is to compare the size first?

I don't think so. Usually, String::compare isn't, and Some containers don't have O(1) length at all.

but, here we can compare size first due to performance reason.

I'm pretty sure we compare string with length first. Also, the length can always be traced for any container.

[peter-jerry-ye]

Here we do dfs first and then compare length, which is weird.

[hackwaly]

Also, the length can always be traced for any container.

The case is Iter::compare, and if we gonna to have something like LazySeq.

[peter-jerry-ye]

Also, the length can always be traced for any container.

The case is Iter::compare, and if we gonna to have something like LazySeq.

Do you mean comparing two Iter? I don't think that's possible with internal iterator, same reason as Iter::zip. As of LazySeq, yeah, that's the only case where we have to compare the elements first. But do we really need that? You might encounter infinite length Seq.

type LazySeq () -> Int?

let a : LazySeq = fn () { Some(5) }
let b : LazySeq = fn () { Some(5) }

let c = a < b // <-- will you ever do that?