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

Apply brace_style to extern blocks #4428

Closed
calebcartwright opened this issue Sep 19, 2020 · 7 comments
Closed

Apply brace_style to extern blocks #4428

calebcartwright opened this issue Sep 19, 2020 · 7 comments
Labels
1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release good first issue Issues up for grabs, also good candidates for new rustfmt contributors hacktoberfest help wanted

Comments

@calebcartwright
Copy link
Member

When brace_style is set to AlwaysNextLine then that should also apply to extern blocks. Until recently this config option was only utilized for items, but was recently updated to apply to unsafe blocks and makes sense to do the same for extern

Some relevant places to check in the code:

Perhaps format_extern, which adds a trailing space after the extern keyword or ABI. We don't want to leave trailing spaces at the end of a line so would need to make sure that there wouldn't be a trailing space left behind when the configured brace_style resulted in the opening brace going on the next line
https://github.com/rust-lang/rustfmt/blob/master/src/formatting/utils.rs#L149

And also format_foreign_mod/format_item

fn format_item(&mut self, item: &Item<'_>) {
self.buffer.push_str(&item.abi);
let snippet = self.snippet(item.span);
let brace_pos = snippet.find_uncommented("{").unwrap();
self.push_str("{");
if !item.body.is_empty() || contains_comment(&snippet[brace_pos..]) {
// FIXME: this skips comments between the extern keyword and the opening
// brace.
self.last_pos = item.span.lo() + BytePos(brace_pos as u32 + 1);

So when brace_style is set to AlwaysNextLine rustfmt would emit:

extern "C"
{
    fn foo(x: i32);
}

otherwise

extern "C" {
    fn foo(x: i32);
}
@calebcartwright calebcartwright added good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted labels Sep 19, 2020
@IceTDrinker
Copy link
Contributor

I'll take a look tomorrow, not going to be super available next week, so unsure for a reasonable ETA

@blesson3
Copy link
Contributor

blesson3 commented Oct 2, 2020

I just opened a PR (#4450) that allows extern blocks to conform to brace_style when set to AlwaysNextLine.

I also made sure that there are no trailing spaces for the extern "C" line after formatted.

@IceTDrinker
Copy link
Contributor

I also have a proposal not based on fixing the strings after the fact, I'll push it on my repo and not open a PR to spam you guys : https://github.com/IceTDrinker/rustfmt/tree/brace_style_always_new_line_extern

I took the liberty to reuse @blesson3 test files

Main issue I see with current extern formatting is comments between extern keyword and ABI string and comment between ABI string and opening bracket. Could be a separate issue

Cheers

@calebcartwright
Copy link
Member Author

Thanks @IceTDrinker! I did go ahead and merge @blesson3's PR (actually didn't see the comments here til after) as it addressed the main issue. Please still feel free to submit your PR if you think we can improve, capture comments, etc.

@blesson3
Copy link
Contributor

This feature has regressed in the master branch. I cannot seem to find where my PR (#4450) changes went. Either this is intentional or somehow those commits were lost. @calebcartwright I recommend this issue be re-opened until this is clarified.

@calebcartwright
Copy link
Member Author

@blesson3 - please see #4801

@calebcartwright calebcartwright added the 1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release label May 16, 2021
@blesson3
Copy link
Contributor

I had not seen that, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release good first issue Issues up for grabs, also good candidates for new rustfmt contributors hacktoberfest help wanted
Projects
None yet
Development

No branches or pull requests

3 participants