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.

Sign up for GitHub

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

Jump to bottom

Add new ThinBox type for 1 stack pointer wide heap allocated trait objects #90066

Merged
merged 1 commit into from
Apr 9, 2022
Merged

Add new ThinBox type for 1 stack pointer wide heap allocated trait objects #90066

merged 1 commit into from
Apr 9, 2022

Conversation

yaahc
Copy link
Member

@yaahc yaahc commented Oct 19, 2021 •
edited
Loading

@rust-highfive
Copy link
Collaborator

r? @kennytm

(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 19, 2021
@yaahc
Copy link
Member Author

yaahc commented Oct 19, 2021

r? @m-ou-se

cc @eddyb

@rust-highfive rust-highfive assigned m-ou-se and unassigned kennytm Oct 19, 2021
@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 19, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@yaahc

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@yaahc

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Oct 20, 2021

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

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 20, 2021
@matthieu-m

This comment has been minimized.

Sign in to view
@yaahc

This comment has been minimized.

Sign in to view
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 10, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@yaahc yaahc force-pushed the thinbox branch 3 times, most recently from 7b4d93b to e08f43c Compare January 11, 2022 20:31
@yaahc
Copy link
Member Author

yaahc commented Feb 18, 2022

r? rust-lang/libs

@yaahc yaahc added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 18, 2022
@yaahc
Copy link
Member Author

yaahc commented Feb 18, 2022

second attempt!

r? rust-lang/libs

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2022
@bors
Copy link
Contributor

bors commented Apr 1, 2022

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

joshtriplett
joshtriplett reviewed Apr 7, 2022
View reviewed changes
library/alloc/src/lib.rs
@@ -152,6 +153,7 @@
#![feature(fundamental)]
#![cfg_attr(not(test), feature(generator_trait))]
#![feature(lang_items)]
#![feature(let_else)]
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@joshtriplett
Copy link
Member

Posted a few comments, and un-resolved one other comment that didn't get addressed.

LGTM otherwise; r=me with those addressed.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@yaahc yaahc mentioned this pull request Apr 8, 2022
Merged
@bors
Copy link
Contributor

bors commented Apr 8, 2022

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

@yaahc @joshtriplett
Add ThinBox type for 1 stack pointer sized heap allocated trait objects
a87a0d0 
Relevant commit messages from squashed history in order:

Add initial version of ThinBox

update test to actually capture failure

swap to middle ptr impl based on matthieu-m's design

Fix stack overflow in debug impl

The previous version would take a `&ThinBox<T>` and deref it once, which
resulted in a no-op and the same type, which it would then print causing
an endless recursion. I've switched to calling `deref` by name to let
method resolution handle deref the correct number of times.

I've also updated the Drop impl for good measure since it seemed like it
could be falling prey to the same bug, and I'll be adding some tests to
verify that the drop is happening correctly.

add test to verify drop is behaving

add doc examples and remove unnecessary Pointee bounds

ThinBox: use NonNull

ThinBox: tests for size

Apply suggestions from code review

Co-authored-by: Alphyr <[email protected]>

use handle_alloc_error and fix drop signature

update niche and size tests

add cfg for allocating APIs

check null before calculating offset

add test for zst and trial usage

prevent optimizer induced ub in drop and cleanup metadata gathering

account for arbitrary size and alignment metadata

Thank you nika and thomcc!

Update library/alloc/src/boxed/thin.rs

Co-authored-by: Josh Triplett <[email protected]>

Update library/alloc/src/boxed/thin.rs

Co-authored-by: Josh Triplett <[email protected]>
@yaahc
Copy link
Member Author

yaahc commented Apr 9, 2022

@bors r=joshtriplett

@bors
Copy link
Contributor

bors commented Apr 9, 2022

📌 Commit a87a0d0 has been approved by joshtriplett

@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 Apr 9, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Apr 9, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 9, 2022
@bors
Auto merge of rust-lang#95835 - Dylan-DPC:rollup-l5mf2ad, r=Dylan-DPC
4bb685e 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#90066 (Add new ThinBox type for 1 stack pointer wide heap allocated trait objects)
 - rust-lang#95374 (assert_uninit_valid: ensure we detect at least arrays of uninhabited types)
 - rust-lang#95599 (Strict provenance lints)
 - rust-lang#95751 (Don't report numeric inference ambiguity when we have previous errors)
 - rust-lang#95764 ([macro_metavar_expr] Add tests to ensure the feature requirement)
 - rust-lang#95787 (reword panic vs result section to remove recoverable vs unrecoverable framing)
 - rust-lang#95797 (Remove explicit delimiter token trees from `Delimited`.)
 - rust-lang#95804 (rustdoc: Fix empty doc comment with backline ICE)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ee8cea8 into rust-lang:master Apr 9, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a1phyr a1phyr a1phyr left review comments

@compiler-errors compiler-errors compiler-errors left review comments

@matthieu-m matthieu-m matthieu-m left review comments

@thomcc thomcc thomcc left review comments

@RalfJung RalfJung RalfJung left review comments

@joshtriplett joshtriplett joshtriplett left review comments

@eddyb eddyb eddyb left review comments

@cuviper cuviper cuviper left review comments

@ben0x539 ben0x539 ben0x539 left review comments

Assignees

@joshtriplett joshtriplett

Labels
S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.