Share implementation of sort methods.

[chescock]

Objective

The various Query::sort() methods have a lot of duplicated code between them, including some unsafe code. Reduce the duplication to make the code easier to read and maintain.

Solution

Extract the duplicated code to a private method, and pass in the sorting strategy as a closure.

Testing

I used cargo-show-asm to verify that the closures were inlined, but I didn't run anything through a profiler. The sort() method itself even had identical assembly before and after this change, although the others did not.

[chescock]

Query::sort_unstable_by was calling sort_by instead of sort_unstable_by. That seemed like a bug, so I changed it. If we don't merge this PR, we should probably make one that fixes this line.

[Victoronz]

Good catch! This is a bug.

[Victoronz]

Do you see a way to preserve whether Entity needs to be wrapped in NeutralOrd?

Since this is no longer public, no need to restrict yourself to impl Trait, you can just use plain generics here.

[chescock]

It seems simpler to use it everywhere. It's a simple newtype wrapper, so it won't affect performance. And the methods that weren't wrapping the Entity don't actually use it during sorting, so it won't affect behavior, either.

[Victoronz]

Fair, it should be okay then!

[Victoronz]

It'd be nice if this could be done without introducing an additonal level of closure, maybe with generics?

Another reason for punting on this was that while these do the same now, that may change when sort caching is introduced.
This might have to look different then.

[chescock]

It'd be nice if this could be done without introducing an additonal level of closure, maybe with generics?

What's wrong with using a closure here? Closures are just syntactic sugar for generics, and I don't see how to make things more clear here with named types. (There's no dynamic dispatch, if that's your worry; this is impl FnOnce, not dyn FnOnce. It all gets inlined.)

[chescock]

Another reason for punting on this was that while these do the same now, that may change when sort caching is introduced.
This might have to look different then.

Oh, I didn't know there were plans for sort caching! This PR certainly isn't urgent, so it can wait if that's happening soon.

Depending on your plans for caching, though, this change could make it easier. I wouldn't expect the caching to vary wildly based on the type of sort, so doing this first could mean that you have to add it to one place instead of six.

[Victoronz]

It'd be nice if this could be done without introducing an additonal level of closure, maybe with generics?

What's wrong with using a closure here? Closures are just syntactic sugar for generics, and I don't see how to make things more clear here with named types. (There's no dynamic dispatch, if that's your worry; this is impl FnOnce, not dyn FnOnce. It all gets inlined.)

While this might generate mildly different code, the main concern here instead would be capability to preserve the information that might be useful for caching: the sort kind, and the sort function used, if there is one (maybe as a fn pointer).

However, caching is not yet in the process of being implemented and is still being discussed in #13464, so how it'll look like is up in the air.

[iiYese]

Another reason for punting on this was that while these do the same now, that may change when sort caching is introduced.

It shouldn't. The public user facing APIs for the cached variants will only allow fn pointers because closures can be stateful & alter how they sort. The cache would have no way of detecting if this state is changed hence they're not allowed. All impl Fns can accept function pointers.

[Victoronz]

It'd be nice if this could be done without introducing an additonal level of closure, maybe with generics?

What's wrong with using a closure here? Closures are just syntactic sugar for generics, and I don't see how to make things more clear here with named types. (There's no dynamic dispatch, if that's your worry; this is impl FnOnce, not dyn FnOnce. It all gets inlined.)

While this might generate mildly different code, the main concern here instead would be capability to preserve the information that might be useful for caching: the sort kind, and the sort function used, if there is one (maybe as a fn pointer).

Actually, the information I am talking about can and would better be passed as an additional parameter to sort_impl if need be, so no longer a concern.