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

Rollup of 5 pull requests #63671

Merged
merged 35 commits into from
Aug 18, 2019
Merged

Rollup of 5 pull requests #63671

merged 35 commits into from
Aug 18, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Aug 17, 2019

Successful merges:

Failed merges:

r? @ghost

sd234678 and others added 30 commits August 16, 2019 10:54
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
Co-Authored-By: Ralf Jung <post@ralfj.de>
Co-Authored-By: Ralf Jung <post@ralfj.de>
Add APIs for uninitialized Box, Rc, and Arc. (Plus get_mut_unchecked)

Assigning `MaybeUninit::<Foo>::uninit()` to a local variable is usually free, even when `size_of::<Foo>()` is large. However, passing it for example to `Arc::new` [causes at least one copy](https://youtu.be/F1AquroPfcI?t=4116) (from the stack to the newly allocated heap memory) even though there is no meaningful data. It is theoretically possible that a Sufficiently Advanced Compiler could optimize this copy away, but this is [reportedly unlikely to happen soon in LLVM](https://youtu.be/F1AquroPfcI?t=5431).

This PR proposes two sets of features:

* Constructors for containers (`Box`, `Rc`, `Arc`) of `MaybeUninit<T>` or `[MaybeUninit<T>]` that do not initialized the data, and unsafe conversions to the known-initialized types (without `MaybeUninit`). The constructors are guaranteed not to make unnecessary copies.

* On `Rc` and `Arc`, an unsafe `get_mut_unchecked` method that provides `&mut T` access without checking the reference count. `Arc::get_mut` involves multiple atomic operations whose cost can be non-trivial. `Rc::get_mut` is less costly, but we add `Rc::get_mut_unchecked` anyway for symmetry with `Arc`.

  These can be useful independently, but they will presumably be typical when the new constructors of `Rc` and `Arc` are used.

  An alternative with a safe API would be to introduce `UniqueRc` and `UniqueArc` types that have the same memory layout as `Rc` and `Arc` (and so zero-cost conversion to them) but are guaranteed to have only one reference. But introducing entire new types feels “heavier” than new constructors on existing types, and initialization of `MaybeUninit<T>` typically requires unsafe code anyway.

Summary of new APIs (all unstable in this PR):

```rust
impl<T> Box<T> { pub fn new_uninit() -> Box<MaybeUninit<T>> {…} }
impl<T> Box<MaybeUninit<T>> { pub unsafe fn assume_init(self) -> Box<T> {…} }
impl<T> Box<[T]> { pub fn new_uninit_slice(len: usize) -> Box<[MaybeUninit<T>]> {…} }
impl<T> Box<[MaybeUninit<T>]> { pub unsafe fn assume_init(self) -> Box<[T]> {…} }

impl<T> Rc<T> { pub fn new_uninit() -> Rc<MaybeUninit<T>> {…} }
impl<T> Rc<MaybeUninit<T>> { pub unsafe fn assume_init(self) -> Rc<T> {…} }
impl<T> Rc<[T]> { pub fn new_uninit_slice(len: usize) -> Rc<[MaybeUninit<T>]> {…} }
impl<T> Rc<[MaybeUninit<T>]> { pub unsafe fn assume_init(self) -> Rc<[T]> {…} }

impl<T> Arc<T> { pub fn new_uninit() -> Arc<MaybeUninit<T>> {…} }
impl<T> Arc<MaybeUninit<T>> { pub unsafe fn assume_init(self) -> Arc<T> {…} }
impl<T> Arc<[T]> { pub fn new_uninit_slice(len: usize) -> Arc<[MaybeUninit<T>]> {…} }
impl<T> Arc<[MaybeUninit<T>]> { pub unsafe fn assume_init(self) -> Arc<[T]> {…} }

impl<T: ?Sized> Rc<T> { pub unsafe fn get_mut_unchecked(this: &mut Self) -> &mut T {…} }
impl<T: ?Sized> Arc<T> { pub unsafe fn get_mut_unchecked(this: &mut Self) -> &mut T {…} }
```
…s-in-src/test-2, r=Centril

Remove meaningless comments in src/test

Moved from rust-lang#63411
Crank up invalid value lint

* Warn against uninit `bool` and `char`.
* Warn against 0-init `NonNull` and friends
* Detect transmute-from-0 as zero-initialization ([seen in the wild](glium/glium#1775 (comment)))
…ewjasper

resolve: Properly integrate derives and `macro_rules` scopes

So,
```rust
#[derive(A, B)]
struct S;

m!();
```
turns into something like
```rust
struct S;

A_placeholder!( struct S; );

B_placeholder!( struct S; );

m!();
```
during expansion.

And for `m!()` its "`macro_rules` scope" (aka "legacy scope") should point to the `B_placeholder` call rather than to the derive container `#[derive(A, B)]`.

`fn build_reduced_graph` now makes sure the legacy scope points to the right thing.
(It's still a mystery for me why this worked before rust-lang#63535.)

Unfortunately, placeholders from derives are currently treated separately from placeholders from other macros and need to be passed as `extra_placeholders` rather than a part of the AST fragment.
That's fixable, but I wanted to keep this PR more minimal to close the regression faster.

Fixes rust-lang#63651
r? @matthewjasper
@Centril
Copy link
Contributor Author

Centril commented Aug 17, 2019

@bors r+ p=5 rollup=never

@bors
Copy link
Contributor

bors commented Aug 17, 2019

📌 Commit 4ec9703 has been approved by Centril

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 17, 2019
@bors
Copy link
Contributor

bors commented Aug 17, 2019

⌛ Testing commit 4ec9703 with merge bd1da18...

bors added a commit that referenced this pull request Aug 17, 2019
Rollup of 5 pull requests

Successful merges:

 - #62451 (Add APIs for uninitialized Box, Rc, and Arc. (Plus get_mut_unchecked))
 - #63487 (Remove meaningless comments in src/test)
 - #63657 (Crank up invalid value lint)
 - #63667 (resolve: Properly integrate derives and `macro_rules` scopes)
 - #63669 (fix typos in mir/interpret)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Aug 18, 2019

☀️ Test successful - checks-azure
Approved by: Centril
Pushing bd1da18 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 18, 2019
@bors bors merged commit 4ec9703 into rust-lang:master Aug 18, 2019
@Centril Centril deleted the rollup-zufavt5 branch August 18, 2019 07:44
@Centril Centril added the rollup A PR which is a rollup label Oct 2, 2019
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. rollup A PR which is a rollup 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.

7 participants