Galloping search for binary_search_util #67948
Conversation
|
r? @zackmdavis (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Btw. I have a quickcheck test to test against master, but that probably shouldn't run with every build. |
There was a problem hiding this comment.
Algorithm looks correct, though I can't say that I can follow it super closely (would likely need to sketch it out on paper to review thoroughly). Since you've tested with quickcheck though and it seems to match my understanding of what it should look like, this seems fine.
r=me with the declarations split onto multiple lines
| } else { | ||
| // We get back *some* element with the given key -- so do | ||
| // a galloping search backwards to find the *first* one. | ||
| let (mut start, mut step, mut previous, size) = (mid, 1, mid, data.len()); |
There was a problem hiding this comment.
Could we declare these on separate lines? My eyes are having trouble correlating initial values with the variable names :)
|
@bors r+ Thanks! |
|
📌 Commit 0e1cd59 has been approved by |
bors
added
S-waiting-on-bors
Galloping search for binary_search_util This is unlikely to improve perf much unless for synthetic benchmarks, but I figure it likely won't hurt either.
Rollup of 6 pull requests Successful merges: - #67494 (Constify more of alloc::Layout) - #67867 (Correctly check for opaque types in `assoc_ty_def`) - #67948 (Galloping search for binary_search_util) - #68045 (Move more of `rustc::lint` into `rustc_lint`) - #68089 (Unstabilize `Vec::remove_item`) - #68108 (Add suggestions when encountering chained comparisons) Failed merges: r? @ghost
This is unlikely to improve perf much unless for synthetic benchmarks, but I figure it likely won't hurt either.