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

Tweak nearest_common_ancestor(). #50649

Merged
merged 1 commit into from
May 18, 2018

Conversation

nnethercote
Copy link
Contributor

  • Remove the "no nearest common ancestor found" case, because it's never
    hit in practise. (This means closure_is_enclosed_by can also be
    removed.)

  • Add a comment about why SmallVec is used for the "seen" structures.

  • Use &Scope instead of Scope to avoid some map() calls.

  • Use any(p) instead of position(p).is_some().

r? @nikomatsakis

- Remove the "no nearest common ancestor found" case, because it's never
  hit in practise. (This means `closure_is_enclosed_by` can also be
  removed.)

- Add a comment about why `SmallVec` is used for the "seen" structures.

- Use `&Scope` instead of `Scope` to avoid some `map()` calls.

- Use `any(p)` instead of `position(p).is_some()`.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 11, 2018
let mut mb = Some(&scope_b);

// A HashSet<Scope> is a more obvious choice for these, but SmallVec is
// faster because the set size is normally small so linear search is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you verify this? It sounds like it should be true but I've run into some surprises because FxHashSet is so fast.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did verify it. The difference was small but noticeable.

@nikomatsakis
Copy link
Contributor

@bors r+

🍑

@bors
Copy link
Contributor

bors commented May 14, 2018

📌 Commit a91f6f7 has been approved by nikomatsakis

@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-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2018
@Mark-Simulacrum
Copy link
Member

@bors rollup

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 17, 2018
…cestor, r=nikomatsakis

Tweak `nearest_common_ancestor()`.

- Remove the "no nearest common ancestor found" case, because it's never
  hit in practise. (This means `closure_is_enclosed_by` can also be
  removed.)

- Add a comment about why `SmallVec` is used for the "seen" structures.

- Use `&Scope` instead of `Scope` to avoid some `map()` calls.

- Use `any(p)` instead of `position(p).is_some()`.

r? @nikomatsakis
bors added a commit that referenced this pull request May 18, 2018
Rollup of 10 pull requests

Successful merges:

 - #50387 (Remove leftover tab in libtest outputs)
 - #50553 (Add Option::xor method)
 - #50610 (Improve format string errors)
 - #50649 (Tweak `nearest_common_ancestor()`.)
 - #50790 (Fix grammar documentation wrt Unicode identifiers)
 - #50791 (Fix null exclusions in grammar docs)
 - #50806 (Add `bless` x.py subcommand for easy ui test replacement)
 - #50818 (Speed up `opt_normalize_projection_type`)
 - #50837 (Revert #49767)
 - #50839 (Make sure people know the book is free oline)

Failed merges:
@bors bors merged commit a91f6f7 into rust-lang:master May 18, 2018
@nnethercote nnethercote deleted the tweak-nearest_common_ancestor branch May 18, 2018 05:50
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 22, 2021
Remove `ScopeTree::closure_tree`

Seems to be dead code since rust-lang#50649.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 22, 2021
Remove `ScopeTree::closure_tree`

Seems to be dead code since rust-lang#50649.
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.

6 participants