impl Ord for params and use unstable sort

[hkBst]

AFAICT we are only sorting to find duplicates, so unstable sort should work fine, and maybe is a tiny bit faster?

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[nnethercote]

How did you find this? I don't know if it's worth adding the PartialOrd/Ord impls.

View changes since this review

[hkBst]

How did you find this?

I was looking at the code in #146980 because clippy triggers on the mutation of the loop variable, and was wondering about the manual Vec/FxHashSet subset (vec.all(set.contains())) and so wandered around a bit... then found myself wondering what kind of non-default-sort was happening. :D

I don't know if it's worth adding the PartialOrd/Ord impls.

I added them, because the sort_by_key(|x| x.0) made it look like it was doing a limited or special sorting different from the default sorting, but element 0 is literally its only element, so it is the default sorting. It seems clearer to me like this, but I'm not sure what the trade-off would be.

[nnethercote]

I think the trade-offs are negligible... it's extremely unlikely to affect performance, and I find readability much the same either way. So it's a question of why bother?

[hkBst]

Well, the way it's currently written, the sort is done with custom cmp, but the dedup uses default eq from partialEq. That inconsistency made me look into what was going on, so I know it can be a distraction. So for consistency I think it would be better if they both go the same way: either both use the same projection to element 0, or both use the default from PartialEq and Ord. My preference is for the latter option, but let me know which way you want to go.

[nnethercote]

Eh, ok.

@bors r+ rollup

[bors]

📌 Commit 431ef03 has been approved by nnethercote

It is now in the queue for this repository.