Uh oh!
There was an error while loading. Please reload this page.
Updated Binary Search docs to show insert into an already sorted list - #61742
Updated Binary Search docs to show insert into an already sorted list#61742ptallo wants to merge 2 commits into
Conversation
rust-highfive
commented
Jun 11, 2019
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
jonas-schievink
left a comment
There was a problem hiding this comment.
Looks good to me, but let's wait for someone on the docs/libs team to look over it. Thanks for the PR :)
ptallo
commented
Jun 11, 2019
No problem! Thanks for the review help, grammar isn't my strong suit! |
rust-highfive
commented
Jun 11, 2019
The job Click to expand the log.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 |
ondj
commented
Jun 12, 2019
Why something like this? ifletErr(pos) = s.binary_search(&num){
s.insert(pos, num);} |
…ed list Resolves: rust-lang#61684 Apply suggestions from code review Co-Authored-By: Jonas Schievink <jonasschievink@gmail.com> Cleanup: Fixed failing tidy checks
ptallo
commented
Jun 12, 2019
was squashing cleanup commits into one commit, and @ondj I believe that would work as well, this way just seemed like it would allow more modification (editing of the OK() or Err() blocks) |
czipperz
commented
Jun 22, 2019
Ping @ptallo |
| /// let num = 14; | ||
| /// | ||
| /// match s.binary_search(&num) { | ||
| /// Ok(_) => {} |
There was a problem hiding this comment.
Hm, I think we should be inserting regardless of finding the item here, so filling out this Ok branch with an insert as well would be good, perhaps even illustrating that by doing something like Ok(idx) | Err(idx) => s.insert(idx, num) in an if let.
There was a problem hiding this comment.
I believe my new commit is more what you were looking for. Do I need to squash the commits or will the merge do that for me?
There was a problem hiding this comment.
You'll need to squash the commits; it also looks like you have a build failure here -- I suspect the ; after the if let.
Mark-Simulacrum
commented
Jul 9, 2019
I'll take over review here for now, left a comment. |
| /// let num = 14; | ||
| /// | ||
| /// let idx = s.binary_search(&num); | ||
| /// if let Ok(idx) | Err(idx) { |
There was a problem hiding this comment.
I believe this is invalid syntax and should be if let Ok(idx) | Err(idx) = idx {
There was a problem hiding this comment.
See error:
error: expected one of `=` or `|`, found `{`
--> slice/mod.rs:1361:27
|
8 | if let Ok(idx) | Err(idx) {
| ^ expected one of `=` or `|` here
| /// | ||
| /// let idx = s.binary_search(&num); | ||
| /// if let Ok(idx) | Err(idx) { | ||
| /// s.insert(idx, num) |
There was a problem hiding this comment.
There have been losed a semicolon
s.insert(idx,num);
totsteps
commented
Jul 30, 2019
ping from triage, @ptallo any updates on this? |
fmckeogh
commented
Aug 6, 2019
tesuji
commented
Aug 10, 2019
I rework in #63442. |
…k-Simulacrum Add an example to show how to insert item to a sorted vec Closesrust-lang#61684 cc rust-lang#61742 r? @Mark-Simulacrum, @jonas-schievink
…k-Simulacrum Add an example to show how to insert item to a sorted vec Closesrust-lang#61684 cc rust-lang#61742 r? @Mark-Simulacrum, @jonas-schievink
Resolves: #61684