- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13.9k
BTreeMap: replace Root with NodeRef<Owned, ...> #78104
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
Conversation
0db2701    to
    591aa08      
    Compare
  
    | I'm going to wait for the changes in #77244 to get rebased away, I think that'll help with review (since we had changed some things upstream in that PR). That said I think I agree that I like this PR's idea. | 
| ☔ The latest upstream changes (presumably #77244) 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:  | 
591aa08    to
    ea4627a      
    Compare
  
    ea4627a    to
    da61221      
    Compare
  
    da61221    to
    b9e727f      
    Compare
  
    b9e727f    to
    2df9277      
    Compare
  
    | @bors r+ | 
| 📌 Commit 2df9277 has been approved by  | 
…mulacrum BTreeMap: ban BoxedNode from Root `NodeRef<marker::Owned, …>` already exists as a representation of root nodes, and it makes more sense to alias `Root` to that than to reuse the space-efficient `BoxedNode` that is oblivious to height, where height is required. r? @Mark-Simulacrum
| this has conflicts: @bors r- | 
| ☔ The latest upstream changes (presumably #78015) 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:  | 
2df9277    to
    7cf9cf9      
    Compare
  
    | Still contains (builds on) #78359. I don't care if they are merged separately, but this one seems more fragile. Also, I'm now thinking it would be better to stop using the Root alias at the node level and move it up to the map level. | 
| 
 Hm, I'm not sure I quite follow, but I think this feels wrong -- roots are useful when e.g. splitting out a subtree which will later be joined in a different place. I'm not sure that the code will care too much about this distinction though. | 
| ⌛ Testing commit 9fca57c with merge 23f0c771fe1e965804082861824289a26677c121... | 
| 💔 Test failed - checks-actions | 
| Not a rotten apple this time, but  PS 🤦♂️ that's almost literally what it says one line lower | 
| @bors retry - hopefully spurious, or alternatively everything is going to fail with this... | 
| ⌛ Testing commit 9fca57c with merge 2e68256e76e4afcad0a5a7f96c99a1fe84637847... | 
| Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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  | 
| 💔 Test failed - checks-actions | 
| @bors retry OpenSSL SSL_read: Connection was reset | 
| ☀️ Test successful - checks-actions | 
…mulacrum BTreeMap: swap the names of NodeRef::new and Root::new_leaf rust-lang#78104 preserved the name of Root::new_leaf to minimize changes, but the resulting names are confusing. r? `@Mark-Simulacrum`
NodeRef<marker::Owned, …>already exists as a representation of root nodes, and it makes more sense to aliasRootto that than to reuse the space-efficientBoxedNodethat is oblivious to height, where height is required.r? @Mark-Simulacrum