[WIP] Add `From<(Vec<T>, C)>` for BinaryHeap a.k.a. flexible `BinaryHeap` #69454

sekineh · 2020-02-25T08:02:34Z

This PR will

add the second type parameter C: Fn(&T, &T) -> Option<Ordering> to the existing BinaryHeap<T>.
add pub struct MaxCmp which can be used as the default C.
- feature gated by binary_heap_max_cmp
add From<(Vec<T>, C)> impl to support generic BinaryHeap<T, C> creation.
- feature gated by binary_heap_from_vec_cmp
remove T: Ord bound where possible.

The features refer to #38886.

Unresolved questions

Comparator type

I used C: Fn(&T, &T) -> Option<Ordering> which is compatible with PartialOrd::partial_cmp() return type.
C: Fn(&T, &T) -> Ordering is more straightforward but it broke some corner-case test.
Should we go with the latter and fix tests instead?

Needs crater run?

While stage 0 unit tests $ ./x.py test src/liballoc --stage 0 run fine, serde was failed to build.

`serde` needs to be patched before it lands?

The below in serde-1.0.99 conflicts with this PR#.
https://github.com/serde-rs/serde/blob/192f5cd647dfb67bfd6158e7234c78c95894c071/serde/src/de/impls.rs#L786-L794

rust-highfive · 2020-02-25T08:02:45Z

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

rust-highfive · 2020-02-25T08:10:50Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-02-25T08:03:19.7743117Z ========================== Starting Command Output ===========================
2020-02-25T08:03:19.7745954Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/accc4588-c2b2-463b-91f5-7aa5bd753620.sh
2020-02-25T08:03:19.7746273Z 
2020-02-25T08:03:19.7750333Z ##[section]Finishing: Disable git automatic line ending conversion
2020-02-25T08:03:19.7774437Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69454/merge to s
2020-02-25T08:03:19.7779290Z Task         : Get sources
2020-02-25T08:03:19.7779669Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-25T08:03:19.7780018Z Version      : 1.0.0
2020-02-25T08:03:19.7780241Z Author       : Microsoft
---
2020-02-25T08:03:22.3426896Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-02-25T08:03:22.3589785Z ##[command]git config gc.auto 0
2020-02-25T08:03:22.3634447Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-02-25T08:03:22.3680783Z ##[command]git config --get-all http.proxy
2020-02-25T08:03:22.3794252Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/69454/merge:refs/remotes/pull/69454/merge
---
2020-02-25T08:10:40.3534391Z     Checking rustc_index v0.0.0 (/checkout/src/librustc_index)
2020-02-25T08:10:41.8875617Z error[E0277]: the trait bound `T: std::cmp::Ord` is not satisfied
2020-02-25T08:10:41.8877206Z    --> /cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.99/src/de/impls.rs:709:21
2020-02-25T08:10:41.8878096Z     |
2020-02-25T08:10:41.8878953Z 690 | / macro_rules! seq_impl {
2020-02-25T08:10:41.8880761Z 691 | |     (
2020-02-25T08:10:41.8882339Z 692 | |         $ty:ident < T $(: $tbound1:ident $(+ $tbound2:ident)*)* $(, $typaram:ident : $bound1:ident $(+ $bound2:ident)*)* >,
2020-02-25T08:10:41.8883739Z 693 | |         $access:ident,
2020-02-25T08:10:41.8884717Z ...   |
2020-02-25T08:10:41.8885676Z 708 | |                 struct SeqVisitor<T $(, $typaram)*> {
2020-02-25T08:10:41.8887060Z     | |                                   - help: consider restricting this bound: `T: std::cmp::Ord`
2020-02-25T08:10:41.8888511Z 709 | |                     marker: PhantomData<$ty<T $(, $typaram)*>>,
2020-02-25T08:10:41.8891223Z ...   |
2020-02-25T08:10:41.8892152Z 779 | |     }
2020-02-25T08:10:41.8893798Z 780 | | }
2020-02-25T08:10:41.8893798Z 780 | | }
2020-02-25T08:10:41.8894548Z     | |_- in this expansion of `seq_impl!`
2020-02-25T08:10:41.8895223Z ...
2020-02-25T08:10:41.8895809Z 787 | / seq_impl!(
2020-02-25T08:10:41.8896637Z 788 | |     BinaryHeap<T: Ord>,
2020-02-25T08:10:41.8897463Z 789 | |     seq,
2020-02-25T08:10:41.8898277Z 790 | |     BinaryHeap::clear,
2020-02-25T08:10:41.8899685Z 793 | |     BinaryHeap::push
2020-02-25T08:10:41.8900681Z 794 | | );
2020-02-25T08:10:41.8901439Z     | |__- in this macro invocation
2020-02-25T08:10:41.8903139Z     |
2020-02-25T08:10:41.8903139Z     |
2020-02-25T08:10:41.8907073Z     = note: required because of the requirements on the impl of `for<'r, 's> std::ops::Fn<(&'r T, &'s T)>` for `std::collections::binary_heap::MaxCmp`
2020-02-25T08:10:41.8909004Z 
2020-02-25T08:10:41.9029511Z error[E0277]: the trait bound `T: std::cmp::Ord` is not satisfied
2020-02-25T08:10:41.9030348Z    --> /cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.99/src/de/impls.rs:746:72
2020-02-25T08:10:41.9031014Z     |
2020-02-25T08:10:41.9031014Z     |
2020-02-25T08:10:41.9031647Z 690 | / macro_rules! seq_impl {
2020-02-25T08:10:41.9032484Z 691 | |     (
2020-02-25T08:10:41.9033748Z 692 | |         $ty:ident < T $(: $tbound1:ident $(+ $tbound2:ident)*)* $(, $typaram:ident : $bound1:ident $(+ $bound2:ident)*)* >,
2020-02-25T08:10:41.9035004Z 693 | |         $access:ident,
2020-02-25T08:10:41.9040906Z ...   |
2020-02-25T08:10:41.9042110Z 746 | |                 struct SeqInPlaceVisitor<'a, T: 'a $(, $typaram: 'a)*>(&'a mut $ty<T $(, $typaram)*>);
2020-02-25T08:10:41.9049804Z     | |                                              |
2020-02-25T08:10:41.9049804Z     | |                                              |
2020-02-25T08:10:41.9051347Z     | |                                              help: consider further restricting this bound: `T: std::cmp::Ord +`
2020-02-25T08:10:41.9053028Z 779 | |     }
2020-02-25T08:10:41.9053723Z 780 | | }
2020-02-25T08:10:41.9053723Z 780 | | }
2020-02-25T08:10:41.9054497Z     | |_- in this expansion of `seq_impl!`
2020-02-25T08:10:41.9055031Z ...
2020-02-25T08:10:41.9055621Z 787 | / seq_impl!(
2020-02-25T08:10:41.9056459Z 788 | |     BinaryHeap<T: Ord>,
2020-02-25T08:10:41.9057261Z 789 | |     seq,
2020-02-25T08:10:41.9058077Z 790 | |     BinaryHeap::clear,
2020-02-25T08:10:41.9059488Z 793 | |     BinaryHeap::push
2020-02-25T08:10:41.9060366Z 794 | | );
2020-02-25T08:10:41.9061206Z     | |__- in this macro invocation
2020-02-25T08:10:41.9061718Z     |
2020-02-25T08:10:41.9061718Z     |
2020-02-25T08:10:41.9062878Z     = note: required because of the requirements on the impl of `for<'r, 's> std::ops::Fn<(&'r T, &'s T)>` for `std::collections::binary_heap::MaxCmp`
2020-02-25T08:10:41.9064453Z 
2020-02-25T08:10:43.0979564Z error: aborting due to 2 previous errors
2020-02-25T08:10:43.0979915Z 
2020-02-25T08:10:43.0987092Z For more information about this error, try `rustc --explain E0277`.
2020-02-25T08:10:43.0987092Z For more information about this error, try `rustc --explain E0277`.
2020-02-25T08:10:43.1026869Z error: could not compile `serde`.
2020-02-25T08:10:43.1027538Z warning: build failed, waiting for other jobs to finish...
2020-02-25T08:10:44.1486644Z error: build failed
2020-02-25T08:10:44.1514543Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "-Zconfig-profile" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--color" "always" "--features" " llvm" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json-render-diagnostics"
2020-02-25T08:10:44.1526039Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
2020-02-25T08:10:44.1526468Z Build completed unsuccessfully in 0:03:33
2020-02-25T08:10:44.1588114Z == clock drift check ==
2020-02-25T08:10:44.1604227Z   local time: Tue Feb 25 08:10:44 UTC 2020
2020-02-25T08:10:44.1604227Z   local time: Tue Feb 25 08:10:44 UTC 2020
2020-02-25T08:10:44.2248733Z   network time: Tue, 25 Feb 2020 08:10:44 GMT
2020-02-25T08:10:44.2252785Z == end clock drift check ==
2020-02-25T08:10:45.4115670Z 
2020-02-25T08:10:45.4203105Z ##[error]Bash exited with code '1'.
2020-02-25T08:10:45.4218283Z ##[section]Finishing: Run build
2020-02-25T08:10:45.4266142Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69454/merge to s
2020-02-25T08:10:45.4270493Z Task         : Get sources
2020-02-25T08:10:45.4270803Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-25T08:10:45.4271214Z Version      : 1.0.0
2020-02-25T08:10:45.4271437Z Author       : Microsoft
2020-02-25T08:10:45.4271437Z Author       : Microsoft
2020-02-25T08:10:45.4271760Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-02-25T08:10:45.4272136Z ==============================================================================
2020-02-25T08:10:45.7771423Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-02-25T08:10:45.7819667Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/69454/merge to s
2020-02-25T08:10:45.7924237Z Cleaning up task key
2020-02-25T08:10:45.7926538Z Start cleaning up orphan processes.
2020-02-25T08:10:45.8150127Z Terminate orphan process: pid (4712) (python)
2020-02-25T08:10:45.8326701Z ##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

sekineh · 2020-02-25T09:35:14Z

As of 9b1ba2a, stage 0 test $ ./x.py test src/liballoc --stage 0 runs fine.

From the build error, it looks like serde-1.0.99 assumes BinaryHeap<T: Ord> and rustc depends on the crate. Still investigating....

rust-highfive · 2020-02-25T11:03:59Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-02-25T10:54:12.0051904Z ========================== Starting Command Output ===========================
2020-02-25T10:54:12.0054223Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/e9e86476-9664-47f1-976f-98fb8a6acf5c.sh
2020-02-25T10:54:12.0054538Z 
2020-02-25T10:54:12.0058554Z ##[section]Finishing: Disable git automatic line ending conversion
2020-02-25T10:54:12.0073612Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69454/merge to s
2020-02-25T10:54:12.0076998Z Task         : Get sources
2020-02-25T10:54:12.0077241Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-25T10:54:12.0077465Z Version      : 1.0.0
2020-02-25T10:54:12.0077614Z Author       : Microsoft
---
2020-02-25T10:54:12.9924902Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-02-25T10:54:12.9928752Z ##[command]git config gc.auto 0
2020-02-25T10:54:12.9931441Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-02-25T10:54:12.9933710Z ##[command]git config --get-all http.proxy
2020-02-25T10:54:12.9938300Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/69454/merge:refs/remotes/pull/69454/merge
---
2020-02-25T10:59:17.2457518Z     Checking rustc_index v0.0.0 (/checkout/src/librustc_index)
2020-02-25T10:59:18.1219306Z error[E0277]: the trait bound `T: std::cmp::Ord` is not satisfied
2020-02-25T10:59:18.1225240Z    --> /cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.99/src/de/impls.rs:709:21
2020-02-25T10:59:18.1225703Z     |
2020-02-25T10:59:18.1226184Z 690 | / macro_rules! seq_impl {
2020-02-25T10:59:18.1226699Z 691 | |     (
2020-02-25T10:59:18.1227519Z 692 | |         $ty:ident < T $(: $tbound1:ident $(+ $tbound2:ident)*)* $(, $typaram:ident : $bound1:ident $(+ $bound2:ident)*)* >,
2020-02-25T10:59:18.1228307Z 693 | |         $access:ident,
2020-02-25T10:59:18.1228775Z ...   |
2020-02-25T10:59:18.1229319Z 708 | |                 struct SeqVisitor<T $(, $typaram)*> {
2020-02-25T10:59:18.1230137Z     | |                                   - help: consider restricting this bound: `T: std::cmp::Ord`
2020-02-25T10:59:18.1231138Z 709 | |                     marker: PhantomData<$ty<T $(, $typaram)*>>,
2020-02-25T10:59:18.1232686Z ...   |
2020-02-25T10:59:18.1233144Z 779 | |     }
2020-02-25T10:59:18.1233627Z 780 | | }
2020-02-25T10:59:18.1233627Z 780 | | }
2020-02-25T10:59:18.1234153Z     | |_- in this expansion of `seq_impl!`
2020-02-25T10:59:18.1234539Z ...
2020-02-25T10:59:18.1234948Z 787 | / seq_impl!(
2020-02-25T10:59:18.1235615Z 788 | |     BinaryHeap<T: Ord>,
2020-02-25T10:59:18.1236172Z 789 | |     seq,
2020-02-25T10:59:18.1236730Z 790 | |     BinaryHeap::clear,
2020-02-25T10:59:18.1237712Z 793 | |     BinaryHeap::push
2020-02-25T10:59:18.1238249Z 794 | | );
2020-02-25T10:59:18.1238777Z     | |__- in this macro invocation
2020-02-25T10:59:18.1239163Z     |
2020-02-25T10:59:18.1239163Z     |
2020-02-25T10:59:18.1239819Z     = note: required because of the requirements on the impl of `for<'r, 's> std::ops::Fn<(&'r T, &'s T)>` for `std::collections::binary_heap::MaxCmp`
2020-02-25T10:59:18.1240759Z 
2020-02-25T10:59:18.1241143Z error[E0277]: the trait bound `T: std::cmp::Ord` is not satisfied
2020-02-25T10:59:18.1241732Z    --> /cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.99/src/de/impls.rs:746:72
2020-02-25T10:59:18.1242129Z     |
2020-02-25T10:59:18.1242129Z     |
2020-02-25T10:59:18.1242571Z 690 | / macro_rules! seq_impl {
2020-02-25T10:59:18.1243100Z 691 | |     (
2020-02-25T10:59:18.1243889Z 692 | |         $ty:ident < T $(: $tbound1:ident $(+ $tbound2:ident)*)* $(, $typaram:ident : $bound1:ident $(+ $bound2:ident)*)* >,
2020-02-25T10:59:18.1244672Z 693 | |         $access:ident,
2020-02-25T10:59:18.1245136Z ...   |
2020-02-25T10:59:18.1245804Z 746 | |                 struct SeqInPlaceVisitor<'a, T: 'a $(, $typaram: 'a)*>(&'a mut $ty<T $(, $typaram)*>);
2020-02-25T10:59:18.1247839Z     | |                                              |
2020-02-25T10:59:18.1247839Z     | |                                              |
2020-02-25T10:59:18.1248641Z     | |                                              help: consider further restricting this bound: `T: std::cmp::Ord +`
2020-02-25T10:59:18.1249801Z 779 | |     }
2020-02-25T10:59:18.1250285Z 780 | | }
2020-02-25T10:59:18.1250285Z 780 | | }
2020-02-25T10:59:18.1250823Z     | |_- in this expansion of `seq_impl!`
2020-02-25T10:59:18.1251248Z ...
2020-02-25T10:59:18.1251659Z 787 | / seq_impl!(
2020-02-25T10:59:18.1252247Z 788 | |     BinaryHeap<T: Ord>,
2020-02-25T10:59:18.1252802Z 789 | |     seq,
2020-02-25T10:59:18.1253364Z 790 | |     BinaryHeap::clear,
2020-02-25T10:59:18.1254606Z 793 | |     BinaryHeap::push
2020-02-25T10:59:18.1255232Z 794 | | );
2020-02-25T10:59:18.1255841Z     | |__- in this macro invocation
2020-02-25T10:59:18.1256289Z     |
2020-02-25T10:59:18.1256289Z     |
2020-02-25T10:59:18.1257061Z     = note: required because of the requirements on the impl of `for<'r, 's> std::ops::Fn<(&'r T, &'s T)>` for `std::collections::binary_heap::MaxCmp`
2020-02-25T10:59:18.1258128Z 
2020-02-25T10:59:18.9126822Z error: aborting due to 2 previous errors
2020-02-25T10:59:18.9127101Z 
2020-02-25T10:59:18.9127507Z For more information about this error, try `rustc --explain E0277`.
2020-02-25T10:59:18.9127507Z For more information about this error, try `rustc --explain E0277`.
2020-02-25T10:59:18.9157279Z error: could not compile `serde`.
2020-02-25T10:59:18.9157785Z warning: build failed, waiting for other jobs to finish...
2020-02-25T10:59:19.2143660Z error: build failed
2020-02-25T10:59:19.2163510Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "-Zconfig-profile" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--color" "always" "--features" " llvm" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json-render-diagnostics"
2020-02-25T10:59:19.2174570Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
2020-02-25T10:59:19.2174842Z Build completed unsuccessfully in 0:02:44
2020-02-25T10:59:19.2214867Z == clock drift check ==
2020-02-25T10:59:19.2227119Z   local time: Tue Feb 25 10:59:19 UTC 2020
2020-02-25T10:59:19.2227119Z   local time: Tue Feb 25 10:59:19 UTC 2020
2020-02-25T10:59:19.3770467Z   network time: Tue, 25 Feb 2020 10:59:19 GMT
2020-02-25T10:59:19.3774964Z == end clock drift check ==
2020-02-25T10:59:20.7468772Z 
2020-02-25T10:59:20.7519490Z ##[error]Bash exited with code '1'.
2020-02-25T10:59:20.7548072Z ##[section]Finishing: Run build
2020-02-25T10:59:20.7578784Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69454/merge to s
2020-02-25T10:59:20.7582692Z Task         : Get sources
2020-02-25T10:59:20.7582930Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-25T10:59:20.7583162Z Version      : 1.0.0
2020-02-25T10:59:20.7583316Z Author       : Microsoft
2020-02-25T10:59:20.7583316Z Author       : Microsoft
2020-02-25T10:59:20.7583566Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-02-25T10:59:20.7583863Z ==============================================================================
2020-02-25T10:59:20.9915298Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-02-25T10:59:20.9947993Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/69454/merge to s
2020-02-25T10:59:21.0019662Z Cleaning up task key
2020-02-25T10:59:21.0020712Z Start cleaning up orphan processes.
2020-02-25T10:59:21.0149059Z Terminate orphan process: pid (4273) (python)
2020-02-25T10:59:21.0247381Z ##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

sekineh · 2020-02-25T11:29:55Z

I locally commented out conflicting impls is in serde:
https://github.com/serde-rs/serde/blob/192f5cd647dfb67bfd6158e7234c78c95894c071/serde/src/de/impls.rs#L786-L794

After that, serde and rustc compiles.
I confirmed ./x.py test src/liballoc --stage 1 succeeded.

Mark-Simulacrum · 2020-02-25T14:49:19Z

I don't personally think this is something we want. Most users should likely be satisfied with a wrapper struct with a similar structure to what you have here, and something more involved can likely be dealt with by wrapping BinaryHeap itself (e.g., CustomBinaryHeap) and internally wrapping as needed. AFAIK, the special casing of the ordering function is quite rare, so this would be a relatively niche extension, too.

I think implementing this in the ecosystem may be the better option to start with; it would (AFAICT) be instantly a stable implementation detail that the struct has a custom comparator with is a pretty high bar to meet.

(To be clear, my opinions are not the library team's who would be responsible for ultimately accepting this. But I believe these are reasonable concerns to have some answer to before bringing it to a wider audience).

cuviper · 2020-02-25T22:17:07Z

I think implementing this in the ecosystem may be the better option to start with;

For example: https://crates.io/crates/binary-heap-plus

sekineh · 2020-02-27T04:51:26Z

Thanks for comments!

I actually first wrote binary-heap-plus and found that .from_vec_cmp() method was the most convenient and foundational method in the flexible heap. (.new_by() and other constructors can be written on top of that. )

So I decided to port .from_vec_cmp() to std as a form of From<(Vec<T>, C)> trait and made this PR.
I thought it's quite minimal, but still, there are issues to be resolved.

I'm OK to record this PR as just a historical attempt and close for now.
For the time being, I'll maintain binary-heap-plus crate instead.

cuviper · 2020-02-27T08:26:33Z

Ah, sorry, I hadn't even made the connection that you were the author of that crate.

FWIW, I do think that's nicely done, but I don't see a clear path for it to directly replace the std heap. For one, the serde conflict seems to be a sign that this would be a breaking change to the API.

sekineh · 2020-02-27T09:43:58Z

Ah, sorry, I hadn't even made the connection that you were the author of that crate.

It was better for me to mention that first.

FWIW, I do think that's nicely done, but I don't see a clear path for it to directly replace the std heap. For one, the serde conflict seems to be a sign that this would be a breaking change to the API.

serde conflict was one of the big surprises, because adding default type parameter is mostly considered compatible. It looks like you can write a lib that depends on, say, number of type parameter of foreign type.

sekineh added 9 commits February 21, 2020 20:41

Replace comparison with comaprator type.

fbb1ad0

Add impl From<(Vec<T>, C)> for BinaryHeap<T, C>

18e21cf

Refactor where C: clause using Compare<T>

5d646d9

test fix

f3808d8

Get rid of Compare<T>

006b14c

loosen T: Ord requirement where possible.

823062c

Optimize From<> impls.

944f1d2

Add dedicated MaxCmp feature name.

99bce97

Add test for re-heapify using boxed closure.

9b1ba2a

rust-highfive assigned Mark-Simulacrum Feb 25, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 25, 2020

sekineh changed the title ~~Add From<(Vec<T>, C)> for BinaryHeap a.k.a. flexible BinaryHeap~~ [WIP] Add From<(Vec<T>, C)> for BinaryHeap a.k.a. flexible BinaryHeap Feb 25, 2020

run ./x.py fmt

108ee03

sekineh closed this Feb 27, 2020

cuviper mentioned this pull request Mar 31, 2020

Tracking issue for potential breaking changes rust-num/num-traits#47

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add `From<(Vec<T>, C)>` for BinaryHeap a.k.a. flexible `BinaryHeap` #69454

[WIP] Add `From<(Vec<T>, C)>` for BinaryHeap a.k.a. flexible `BinaryHeap` #69454

sekineh commented Feb 25, 2020 •

edited

Loading

rust-highfive commented Feb 25, 2020

rust-highfive commented Feb 25, 2020

sekineh commented Feb 25, 2020

rust-highfive commented Feb 25, 2020

sekineh commented Feb 25, 2020 •

edited

Loading

Mark-Simulacrum commented Feb 25, 2020

cuviper commented Feb 25, 2020

sekineh commented Feb 27, 2020 •

edited

Loading

cuviper commented Feb 27, 2020

sekineh commented Feb 27, 2020

[WIP] Add From<(Vec<T>, C)> for BinaryHeap a.k.a. flexible BinaryHeap #69454

[WIP] Add From<(Vec<T>, C)> for BinaryHeap a.k.a. flexible BinaryHeap #69454

Conversation

sekineh commented Feb 25, 2020 • edited Loading

Unresolved questions

Comparator type

Needs crater run?

serde needs to be patched before it lands?

rust-highfive commented Feb 25, 2020

rust-highfive commented Feb 25, 2020

sekineh commented Feb 25, 2020

rust-highfive commented Feb 25, 2020

sekineh commented Feb 25, 2020 • edited Loading

Mark-Simulacrum commented Feb 25, 2020

cuviper commented Feb 25, 2020

sekineh commented Feb 27, 2020 • edited Loading

cuviper commented Feb 27, 2020

sekineh commented Feb 27, 2020

[WIP] Add `From<(Vec<T>, C)>` for BinaryHeap a.k.a. flexible `BinaryHeap` #69454

[WIP] Add `From<(Vec<T>, C)>` for BinaryHeap a.k.a. flexible `BinaryHeap` #69454

sekineh commented Feb 25, 2020 •

edited

Loading

`serde` needs to be patched before it lands?

sekineh commented Feb 25, 2020 •

edited

Loading

sekineh commented Feb 27, 2020 •

edited

Loading