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

1.x: Backport fixes around deleting pieces of extern functions #4562

Merged

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Nov 29, 2020

This PR is a clean backport of the 2 commits from #4291 and 1 commit from #4314, followed by 1 new commit to delete some test cases from #4291 involving async fn which fail on 1.x due to difference in default edition. #4291 is adequately tested by almost identical signatures using const fn and unsafe fn (the latter being what I actually care about) instead of async.

Backport motivated by the broken behavior being quite problematic for https://github.com/dtolnay/cxx.

topecongiro and others added 4 commits November 28, 2020 18:44
Closes rust-lang#4288

And we get to drop a method, which I think is a win :)
A code like

```rust
extern "C" {
    fn f() {
        fn g() {}
    }
}
```

is incorrect and does not compile. Today rustfmt formats this in a way
that is correct:

```rust
extern "C" {
    fn f();
}
```

But this loses information, and doesn't have to be done because we know
the content of the block if it is present. During development I don't
think rustfmt should drop the block in this context.

Closes rust-lang#4313
Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for this and good timing as I was just prepping release notes for v1.4.28

@calebcartwright calebcartwright merged commit e7ecdc1 into rust-lang:rustfmt-1.4.28 Nov 29, 2020
@dtolnay dtolnay deleted the unsafe-extern branch November 29, 2020 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants