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

BTreeMap: fix pointer provenance rules in underfullness #78631

Merged
merged 1 commit into from
Nov 16, 2020

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Nov 1, 2020

Continuing on #78480, and for readability, and possibly for performance: avoid aliasing when handling underfull nodes, and consolidate the code doing that. In particular:

  • Avoid the rather explicit aliasing for internal nodes in remove_kv_tracking.
  • Climb down to the root to handle underfull nodes using a reborrowed handle, rather than one copied with ptr::read, before resuming on the leaf level.
  • Integrate the code tracking leaf edge position into the functions performing changes, rather than bolting it on.

r? @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 1, 2020
@ssomers ssomers force-pushed the btree-alias_for_underfull branch 3 times, most recently from 07adb06 to 33201e3 Compare November 4, 2020 13:45
@Mark-Simulacrum
Copy link
Member

r=me on the last commit (I did not review the first two, but I believe they have been previously reviewed, so that shouldn't be a concern). Ping me when those are rebased away after the previous PRs land, please.

@Mark-Simulacrum Mark-Simulacrum 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 Nov 8, 2020
@bors
Copy link
Contributor

bors commented Nov 9, 2020

☔ The latest upstream changes (presumably #78889) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@ssomers ssomers force-pushed the btree-alias_for_underfull branch 2 times, most recently from 7a2fe72 to b6c26c4 Compare November 9, 2020 09:50
@ssomers
Copy link
Contributor Author

ssomers commented Nov 9, 2020

I tweaked the name and comments of choose_balancing_contextchoose_parent_kv

@ssomers
Copy link
Contributor Author

ssomers commented Nov 10, 2020

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Also, github doesn't seem to process commits.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 10, 2020
@Mark-Simulacrum
Copy link
Member

First commit indeed seems to be on master, not sure why GitHub is showing it. Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Nov 10, 2020

📌 Commit b6c26c4 has been approved by Mark-Simulacrum

@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 Nov 10, 2020
@ssomers
Copy link
Contributor Author

ssomers commented Nov 11, 2020

The commit you approved is the one GitHub showed me too, but I had (tried to) push a rebase on the newest master. Now GitHub sees that rebase commit, but somehow it's still targeting the old master. While the branch of this PR looks perfectly normal in GitHub itself: one commit on a recent master.

I doubt bors will attempt to take off in this state, so trying to wake up GitHub by pushing an even fresher rebase.

@ssomers
Copy link
Contributor Author

ssomers commented Nov 11, 2020

Aha, just one commit instead of 35 now.

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 11, 2020

📌 Commit 5a129ac35cd7bdb64f762db4eeade48223ebf331 has been approved by Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Nov 12, 2020

☔ The latest upstream changes (presumably #78956) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 12, 2020
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 12, 2020
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Nov 14, 2020

📌 Commit 4cfa5bd has been approved by Mark-Simulacrum

@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 Nov 14, 2020
@bors
Copy link
Contributor

bors commented Nov 15, 2020

⌛ Testing commit 4cfa5bd with merge 3e5a681469d8559e2bb5f9f384487c960db92d72...

@bors
Copy link
Contributor

bors commented Nov 15, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 15, 2020
@Mark-Simulacrum
Copy link
Member

@bors retry

@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 Nov 15, 2020
@bors
Copy link
Contributor

bors commented Nov 15, 2020

⌛ Testing commit 4cfa5bd with merge 1cf9258ce60a03dbbb4589d2df424dc1e11db52c...

@bors
Copy link
Contributor

bors commented Nov 15, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 15, 2020
@Mark-Simulacrum
Copy link
Member

@bors retry

@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 Nov 15, 2020
@bors
Copy link
Contributor

bors commented Nov 16, 2020

⌛ Testing commit 4cfa5bd with merge f5230fb...

@bors
Copy link
Contributor

bors commented Nov 16, 2020

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing f5230fb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 16, 2020
@bors bors merged commit f5230fb into rust-lang:master Nov 16, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 16, 2020
@ssomers ssomers deleted the btree-alias_for_underfull branch November 16, 2020 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

5 participants