WIP comparators for pqueue

[Gankra]

Rough draft of implementing comparators. WIP, just looking for general design discussion. Do not merge.

Notes:

• First part of the file should really be moved to a separate module, maybe where Ord is defined? Realistically I'd like to migrate a lot of std away from requiring Ord, and instead requiring a Comparator (the former being trivial to move to the latter with NaturalOrdering and RevOrdering).
• I tried using default_type_params, but it never seemed to do anything meaningful.
• I tried making comparators impl Fn, but as far as I can tell, the type they'd have to impl would be be Fn<(&T, &T), Ordering>, but the references need to have lifetimes, and I see no sane way to do that generically? Just feeding in 'a,'b everywhere certainly didn't work.
• I made two "I don't care about comparators" constructors min_heap and max_heap as simply public functions, because they didn't make a lot of sense as static methods.
• I made max_heap the default behaviour to match previous behaviour, but now that they're trivially interchangeable, I recommend min_heap be the default, since that matches sorting more closely.
• Comparators are currently overengineered for this use case, (they could just have a single lt method), but again I'd like to refactor them out into a separate module and use them more everywhere. In that context they should return a proper Ord. Might be some overhead right now from doing cmp(x,y) == Greater, rather than gt(x, y).
• Comparators could also provide lt/gt/eq/neq automatically; desirable?
• Weirdly Extend is demanding that the comparator be Default, even though I see no reason for this to be the case.
• Rust seems very bad at inferring the type the comparator wraps, even when it should be unambiguous from context; see: tests.test_iterator
• Could introduce even more combinatoric explosion in the constructors with e.g. from_vec_natural if desired.
• Tests have been rewritten to pass, but no new tests have been added. Similarly docs have not been updated. Didn't consider it a valuable use of time since this is WIP.

[Gankra]

@treeman @thestinger Any opinions on this?

[treeman]

Ah sorry @gankro, been a bit busy. Here are some thoughts:

• Generic approach to comparators seems fine. Similar to what C++ does with their priority queue.
• Does it make sense for RevOrdering to reverse Comparator or should it simply reverse Ord?
• Really like the min_heap, max_heap constructors.
• For ease of use, do we want min_heap_with_* etc? To abstract away explicit comparators? I'm not really sure, but we should probably go with the way easiest for the users.

Generally the approach seems fine. Good job!

Another idea could be to specify PriorityQueue as a trait, and make MinHeap and MaxHeap (possibly other names) which implement the trait. But it seems like the only thing we would gain by going that route would be to avoid one extra typename.

And later on when we standardize the library more, maybe Comparator could be useful for other structures? TreeSet and TreeMap comes to mind.

[Gankra]

Thanks for the feedback, @treeman!

• With RevOrdering I'm favouring composability in the same way Iterators work. I want to eventually move this out into a common area so people can write their own Comparators, and this will wrap any comparator to reverse it. Maybe I should make rev a method on all Comparators? And expose the full Cmp interface while I'm at it?
• I don't have any strong feeling on min/max_heap_with_*. I agree it would be useful. Don't know if the bloat is justified.
• PriorityQueue should definitely become a trait eventually, but I don't think this is the PR for that, though.

[huonw]

FWIW, I think this should be postponed until we have reasonable unboxed closures; in particular #15067.

[Gankra]

@huonw: I agree that unboxed closures change this, and we should wait. Are there many open questions about how unboxed closures will behave/look, or is unboxed closures basically done?

Assuming they take the from described in the RFC, I propose that the Comparator trait is still desirable for the following reasons:

• Clarity: cmp.ge(a, b) is much clearer to me than cmp(a, b) != Less
• Efficiency: It's possible that the individual operations could be written more efficiently than simply wrappers around cmp. If the comparator is being used as a building block for a collection, this overhead could be significant.
• Adapters: we can add a .rev() method to a Comparator Trait in the same vein as Iterators
• Nothing lost: It's easy enough to write a Comparator genericized over an unboxed closure, to permit easy conversion

[Gankra]

@huonw should this be closed, or...?

[huonw]

Yeah, I'm thinking we should close to avoid leaving this in the queue while it waits for unboxed closures. (Thanks for the work!)