Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace the hand-written binary search with the library one #65327

Merged
merged 2 commits into from
Oct 13, 2019

Conversation

guanqun
Copy link
Contributor

@guanqun guanqun commented Oct 12, 2019

No description provided.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2019
@petrochenkov
Copy link
Contributor

lookup_source_file_idx is a very weird function that conflates successful and unsuccessful lookups.

Looks like the previous version assumed that unsuccessful lookups cannot happen, but turns out they do happen (I've just checked) and the function returns a garbage index in this case.
I need to investigate a bit.

@petrochenkov
Copy link
Contributor

Ah, sorry, I misunderstood how this works, we are searching through intervals rather than precise values, of course the search will almost always return Err.

@petrochenkov
Copy link
Contributor

Could you compress the function a bit, like

    pub fn lookup_source_file_idx(&self, pos: BytePos) -> usize {
        self.files.borrow().source_files.binary_search_by_key(&pos, |key| key.start_pos)
            .unwrap_or_else(|p| p - 1)
    }

The asserts don't make much sense here, we'll get a panic one way or another anyway if the number of files is zero.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 12, 2019
@guanqun
Copy link
Contributor Author

guanqun commented Oct 12, 2019

Thank you @petrochenkov for the review and the double check. I just pushed a new commit to compress the function as you suggested. Please help review again, thanks!

@petrochenkov
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 12, 2019

📌 Commit 63cb2fa has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 12, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 13, 2019
…r=petrochenkov

replace the hand-written binary search with the library one
bors added a commit that referenced this pull request Oct 13, 2019
Rollup of 13 pull requests

Successful merges:

 - #65039 (Document missing deny by default lints)
 - #65069 (Implement Clone::clone_from for VecDeque)
 - #65165 (Improve docs on some char boolean methods)
 - #65248 (Suggest `if let` on `let` refutable binding)
 - #65250 (resolve: fix error title regarding private constructors)
 - #65295 (Move diagnostics code out of the critical path)
 - #65320 (Report `CONST_ERR` lint in external macros)
 - #65327 (replace the hand-written binary search with the library one)
 - #65339 (do not reference LLVM for our concurrency memory model)
 - #65357 (syntax: simplify maybe_annotate_with_ascription)
 - #65358 (simplify maybe_stage_features)
 - #65359 (simplify integer_lit)
 - #65360 (mbe: reduce panictry! uses.)

Failed merges:

r? @ghost
@bors bors merged commit 63cb2fa into rust-lang:master Oct 13, 2019
@guanqun guanqun deleted the remove-hand-binary-search branch October 13, 2019 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants