Base.Ordering: do not ever discard order parameter

[tkluck]

Prior to this commit, the order parameter was discarded if either by or order was passed. However, there is a sane interpretation for most of these combinations, and we should use that interpretation for least surprise.

The one case that doesn't have an obvious interpretation is when a non-trivial order is passed together with an lt function; in that case it is better to error out rather than let it pass silently.

For context, the reason why I'm interested in this is that I'm using custom subtypes of Base.Order.Ordering to represent monomial orders in PolynomialRings.jl. It can be very useful to pass e.g. by=first together with order=@degrevlex(x > y).

This also cleans up a seemingly unused function ordtype.

[timholy]

Very nice. You've got a bootstrap issue that needs solving (Revise hides such issues, sadly you'll have to build from scratch), but otherwise this looks good to me.

You might be able to solve it by switching from == to ===.

[tkluck]

@timholy thanks for noticing the issue and for pointing out how to solve it :) Let's see what buildbot says now.

[timholy]

Since this is technically breaking, let's see what PkgEval says:

@nanosoldier runtests(ALL, vs=":master")

[nanosoldier]

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

[tkluck]

The nanosoldier build reveals issues with ~20-30 packages, broadly in two classes:

• the biggest class is those depending on SortingAlgorithms.jl which in turn relies on the removed function Base.Order.ordtype.
• a few packages depend on CuArrays.jl which uses Base.Order.By(....) directly

The second case is easy to solve: I'll add a backward-compatible constructor

Base.Order.By(key, order=Forward)

As for the first case, bringin back Base.Order.ordtype seems like the easy thing to do. OTOH, for future maintainability it would be better if ordtype would just move to be adjacent to where SortingAlgorithms.jl is doing its Order-juggling. There's no testing for it in Base and it was covered by a shady TODO comment before I removed it.

Having said that, the prudent thing to do is to just leave it in... backwards compatibility is pretty important for a language.

[timholy]

@nanosoldier runtests(ALL, vs=":master")

If this comes out clean, before merging it might be worth adding # TODO: deprecate near Base.Order.ordtype. I'd suggest moving it to deprecated.jl but that's a different module. Tagging it with that kind of flag makes it more likely it will be discovered and dealt with for Julia 2.0.

[tkluck]

it might be worth adding # TODO: deprecate near Base.Order.ordtype. I'd suggest moving it to deprecated.jl but that's a different module. Tagging it with that kind of flag makes it more likely it will be discovered and dealt with for Julia 2.0.

I could even add an if VERSION < v"2.0-", maybe?

[nanosoldier]

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

[tkluck]

Just checked the logs from nanosoldier's failures. It's only 8 now and no smoking guns pointing at this change. I think this is safe to merge.

What's you opinion on an if VERSION < v"2.0-" guard for the ordtype definitions? Then no-one has to worry about it in the future.

[timholy]

I think this is safe to merge.

Agreed. Thanks for your care here.

What's you opinion on an if VERSION < v"2.0-" guard for the ordtype definitions? Then no-one has to worry about it in the future.

That seems like a good idea to me. Once you add let's merge this.

[tkluck]

Bootstrapping... I'm checking if I can get VERSION and v"...." to work at bootstrap time and will update here.

[tkluck]

Found a way but it involves ccall. I'll leave it to your judgment whether the ugly code is still better than hoping someone remembers to deprecate it in the future.

[StefanKarpinski]

Possibly because Cint isn't defined yet? Using Int32 probably works because of little-endianness: if the return value is actually Int64 but the caller thinks its Int32 on a little-endian system, the value is right. Would fail on a big-endian system, although, of course, we don't support any of those.

[tkluck]

Possibly because Cint isn't defined yet?

Yes, that's probably it. I didn't attempt to re-shuffle the bootstrap order although it would be nice to have VERSION comparisons and safe ccall.

Would fail on a big-endian system, although, of course, we don't support any of those.

Do we support any platforms where Cint is different from Int32 at all? I can only find

const Cint = Int32

in the source. If we do, your analysis about endian-ness is sound.

[StefanKarpinski]

That's a good point, I was thinking of long which is 32/64-bit. I guess I haven't been doing much C programming for a while now 😄

[tkluck]

This is probably good to merge; the buildbot/tester_win32 failure happens during setup stage and seems completely unrelated.

[timholy]

Very impressive work, @tkluck, thanks!