use Ord for nub

[matthewleon]

On JS engines where Array.push is amortized O(1) (I believe this to be common), this should be amortized O(n*logn).

[matthewleon]

Actually this should always be O(n*logn), as the array sort should provide the upper bound.

[matthewleon]

Thought of a simpler technique, will update this.

[matthewleon]

ready for review

[hdgarrood]

This is great, but I would like to preserve the property that nub xs == xs whenever xs has no duplicates. Perhaps it’s not too difficult to modify this approach so that this property is preserved, by doing something like mapWithIndex (flip Tuple) before sorting, and sorting by snd again at the end?

[hdgarrood]

Also if we don’t have a test for that property, I’d quite like to add one. Doesn’t have to be via quickcheck; if dependencies make quickcheck difficult that’s fine, we can just test it on some hardcoded set of cases.

[hdgarrood]

Could you reference the original issue in the commit as well please?

[garyb]

Very much agreed w/preserving the original order!

[matthewleon]

Will use this if merged: #130

[matthewleon]

I've incorporated the suggestions above. This is ready for review.

[matthewleon]

Actually, I'll add nubEq on this one as well. Why not?

[matthewleon]

Okay, ready for review :).

[hdgarrood]

I just realised, if nub has an Ord constraint, then nubBy should really take an a -> a -> Ordering, so that nub = nubBy compare. We could rename the current nubBy to nubEqBy.

[matthewleon]

nubBy should really take an a -> a -> Ordering, so that nub = nubBy compare. We could rename the current nubBy to nubEqBy.

Done.

[hdgarrood]

Looks great! Do you know what the big-O for this new implementation should be?

Since this is breaking I think it makes sense to merge this as part of the 0.12 updates.

[matthewleon]

Do you know what the big-O for this new implementation should be?

Your upper bound should be set by sort, which uses FFI. For the common case in which you're on a major JS VM, this is O(nlogn).

Hopefully my thinking here is correct. Should I add a comment to this effect?

[matthewleon]

Do you know what the big-O for this new implementation should be?

I should add that if for some reason your FFI'd STArray.push has some kind of pathological complexity, that would of course bring you potentially above O(nlogn). But that seems like a really fringe case.

[hdgarrood]

Sounds good. I think we can assume that push is amortized O(1) (even though 5 minutes of googling was kind of inconclusive).

[kritzcreek]

I merged this by hand into the compiler/0.12 branch. Thanks!