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

Implement unstable ffi_return_twice attribute #58315

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 8, 2019

This PR implements RFC2633

r? @eddyb

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

gnzlbg commented Feb 9, 2019

I'd prefer to enable this only for extern functions. Is there an easy way to error if this attribute is applied to something else?

EDIT: done, I've submitted an RFC for this.

@gnzlbg gnzlbg force-pushed the gnzlbg:returns_twice branch from 7ba1bd9 to 8b8d9d0 Feb 9, 2019

@gnzlbg gnzlbg changed the title Implement unstable return_twice attribute Implement unstable ffi_return_twice attribute Feb 9, 2019

@gnzlbg gnzlbg force-pushed the gnzlbg:returns_twice branch from 8b8d9d0 to bc9bfc7 Feb 9, 2019

@gnzlbg gnzlbg referenced this pull request Feb 9, 2019

Open

#[ffi_returns_twice] #2633

gnzlbg added some commits Feb 9, 2019

@gnzlbg gnzlbg force-pushed the gnzlbg:returns_twice branch from d46a78c to bf2c5d3 Feb 10, 2019

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Feb 14, 2019

@rust-highfive rust-highfive assigned alexcrichton and unassigned eddyb Feb 14, 2019

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented Feb 14, 2019

Code LGTM. Does anyone have a strong opinion on whether to merge this now or wait how the RFC plays out?

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Feb 14, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 14, 2019

r=me on the technical side of things, so for language/rfc/whatnot

r? @RalfJung

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Feb 14, 2019

Uh, sorry, I don't have any skin in this. It's all FFI stuff, which I know little about, and from my understanding it's mostly about taming LLVM's optimizer, which I know even less about.

Functions not being well-nested does have some deep consequences, but I guess there's no way we are going to rule this out entirely.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Feb 14, 2019

Oh, and don't we usually wait until the RFC gets accepted before implementing it? Like, what if we land this and then the RFC gets rejected?

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

gnzlbg commented Feb 14, 2019

Like, what if we land this and then the RFC gets rejected?

We remove the feature and call it a day. AFAICT many unstable features are implemented all the time without nobody ever talking about actually RFCing the stuff. I want to use this in stable Rust at some point, so for that it definitely needs to go properly through the process (and the design space here is small, iff the intend is to do what C does in C FFI at least).

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Feb 14, 2019

shrug

@bors r=alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 14, 2019

📌 Commit d0fddd3 has been approved by alexcrichton

bors added a commit that referenced this pull request Feb 15, 2019

Auto merge of #58315 - gnzlbg:returns_twice, r=alexcrichton
Implement unstable ffi_return_twice attribute

This PR implements [RFC2633](rust-lang/rfcs#2633)

r? @eddyb
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 15, 2019

⌛️ Testing commit d0fddd3 with merge aeb7d5d...

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 15, 2019

💔 Test failed - checks-travis

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 15, 2019

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Feb 15, 2019

@bors retry -- apparently we can't clone the repo anymore on macOS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment