Fix issue 15530: multiSort forbids any SwapStrategy different from un…

[jamesragray]

…stable.

[dlang-bot]

Thanks for your pull request and interest in making D better, @jamesragray! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	15530	major	multiSort forbids any SwapStrategy different from unstable

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8569"

[RazvanN7]

Hi @jamesragray ! Thanks for your pull request. Unfortunately, I don't think this is the right solution. The underlying issue is that partition3 does not support the stable swapping strategy. I suggest fixing partition3 to accept the stable swap strategy.

[jamesragray]

Hi @jamesragray ! Thanks for your pull request. Unfortunately, I don't think this is the right solution. The underlying issue is that partition3 does not support the stable swapping strategy. I suggest fixing partition3 to accept the stable swap strategy.

@RazvanN7 I agree that what I did isn't ideal, but (a) it produces a stable multi sort (b) it is better in my opinion than keeping the broken state (for more than six years). Wouldn't it be better to accept this trivial change and then open a new issue (saying multisort stable is too slow if it really is)?

[pbackus]

Won't this give the wrong answer when the elements in the input range are not already correctly ordered by the 2nd comparison function?

For example, in the new unit test, what happens if you swap Point(1, 1) and Points(1, 2) in pts1?

[jamesragray]

You are correct, line 1582 should be:

sort!(multiSortPredFun!(Range,funs), ss)(r);

I will fix it later today (and split it based on funs.length)

[RazvanN7]

@RazvanN7 I agree that what I did isn't ideal, but (a) it produces a stable multi sort (b) it is better in my opinion than keeping the broken state (for more than six years). Wouldn't it be better to accept this trivial change and then open a new issue (saying multisort stable is too slow if it really is)?

@jamesragray The fact that this is an old issue isn't a motivation to accept a hacky fix. The problem is that if we accept small fixes like this one day we will end up with tons of such cases that will ultimately add to the burden of maintaining the code. As Walter has said in the past "the small crabs that accumulate will eventually sink the ship". So, I, personally, don't agree with this addition. However, I will not block its addition if others accept it.

[jamesragray]

@RazvanN7 I agree putting bad code into phobos just to fix an issue is wrong. I guess what we don't agree about is that although this is simple change I don't consider it to be hacky. It simply forwards multisort stable to sort stable with the approriate predicate. On the other hand I have implemented a stable version of partion3 here: #8583. So when I have time I will benchmark this forwarding approach against using the stable partition3 via the current implementation of multisort.