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

rustdoc: Cleanup ABI rendering #33151

Merged
merged 2 commits into from
Apr 28, 2016
Merged

rustdoc: Cleanup ABI rendering #33151

merged 2 commits into from
Apr 28, 2016

Conversation

ollie27
Copy link
Member

@ollie27 ollie27 commented Apr 22, 2016

Use a common method for rendering extern "<abi>".

This now consistently shows extern fn rather than extern "C" fn.

Use a common method for rendering `extern "<abi>"`.

This now consistently shows `extern "C" fn` rather than just `extern fn`.
@rust-highfive
Copy link
Collaborator

r? @steveklabnik

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

I personally prefer showing extern fn wherever possible as the "C" is just redundant and doesn't need to get typed

@ollie27
Copy link
Member Author

ollie27 commented Apr 22, 2016

rustfmt adds "C" by default and I think it's worth being consistent with that.

@alexcrichton
Copy link
Member

I agree that the two should be consistent, but I would personally consider rustfmt as the one that should change in this regard.

@alexcrichton
Copy link
Member

cc @rust-lang/tools

@ollie27
Copy link
Member Author

ollie27 commented Apr 22, 2016

It was discussed here: rust-lang/rustfmt#451.

@alexcrichton
Copy link
Member

Looks like rustfmt changed to elide "C"?

@mitaa
Copy link
Contributor

mitaa commented Apr 23, 2016

Seems like the default for force_explicit_abi is set to true.
https://github.com/rust-lang-nursery/rustfmt/pull/950/files#diff-6d8a5e2104954a026de2bfe2b936aea4R331

@nrc
Copy link
Member

nrc commented Apr 23, 2016

I've been meaning to canvas opinions on what rustfmt should do here. I don't have a strong opinion.

@steveklabnik
Copy link
Member

I used to put it in, but lately have switched to taking it out. I don't feel super strongly but would lean towards not including it.

On Apr 23, 2016, 12:21 +0300, Nick Cameronnotifications@github.com, wrote:

I've been meaning to canvas opinions on what rustfmt should do here. I don't have a strong opinion.


You are receiving this because you were mentioned.
Reply to this email directly orview it on GitHub(#33151 (comment))

@steveklabnik
Copy link
Member

So, what is the conclusion here? That we're not going to merge this?

@ollie27
Copy link
Member Author

ollie27 commented Apr 26, 2016

I can change this to hide the "C" if that's what people want. After this patch changing it is only a one line change (+ fixing tests) anyway.

The point of this patch was consistency as rustdoc currently uses both styles in different places. For example https://doc.rust-lang.org/1.8.0/libc/fn.atexit.html has both on the same line.

@steveklabnik
Copy link
Member

Ah, right, sorry. Consistency is certainly good.

On Apr 26, 2016, 11:32 -0400, Oliver Middletonnotifications@github.com, wrote:

I can change this to hide the"C"if that's what people want. After this patch changing it is only a one line change (+ fixing tests) anyway.

The point of this patch was consistency as rustdoc currently uses both styles in different places. For examplehttps://doc.rust-lang.org/1.8.0/libc/fn.atexit.htmlhas both on the same line.


You are receiving this because you were mentioned.
Reply to this email directly orview it on GitHub(#33151 (comment))

@alexcrichton
Copy link
Member

Yeah I'd be fine with just removing the rendering of "C" elsewhere

@ollie27
Copy link
Member Author

ollie27 commented Apr 27, 2016

I've removed the "C".

@alexcrichton
Copy link
Member

@bors: r+ 48aabbd

Thanks @ollie27

@bors
Copy link
Contributor

bors commented Apr 28, 2016

⌛ Testing commit 48aabbd with merge cf3970a...

bors added a commit that referenced this pull request Apr 28, 2016
rustdoc: Cleanup ABI rendering

Use a common method for rendering `extern "<abi>"`.

This now consistently shows `extern fn` rather than `extern "C" fn`.
@bors bors mentioned this pull request Apr 28, 2016
@bors bors merged commit 48aabbd into rust-lang:master Apr 28, 2016
@ollie27 ollie27 deleted the rustdoc_abi branch May 2, 2016 18:06
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.

7 participants