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

Feature gate extern fn methods #1429

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
8 participants
@Aatch
Copy link
Contributor

Aatch commented Dec 27, 2015

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Dec 27, 2015

SGTM.

@rphmeier

This comment has been minimized.

Copy link

rphmeier commented Dec 27, 2015

This seems very reasonable, but I'd be interested in seeing the fallout. Are there a lot of bindings to C libraries which use this feature?

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Dec 27, 2015

I know we have received at least one bug report about this feature in conjunction with no_mangle once our symbol duplication detection has landed.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 29, 2015

Doesn't this RFC effectively break all Rust crates that wrap C libraries, e.g. https://crates.io/crates/openssl-sys/ and https://crates.io/crates/libusb-sys/ and many more?

@remram44

This comment has been minimized.

Copy link

remram44 commented Dec 29, 2015

@RalfJung No it doesn't. This RFC only aim to feature-gate extern methods, not functions.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 29, 2015

Oh, I see :)

@nrc nrc added the T-lang label Jan 4, 2016

@nikomatsakis nikomatsakis self-assigned this Jan 7, 2016

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 7, 2016

@Aatch do you have an impl? if you can whip one up, I can do a crater run, feels like necessary data before we make a decision one way or the other. But I'm inclined to 👍 here.

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Apr 7, 2016

Vaguely related, should rustfmt change extern to extern "C" or vice versa: rust-lang/rustfmt#451

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Apr 7, 2016

And +1 for a crater run

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Apr 8, 2016

This is no longer necessary AFAIK, all methods should be compatible with all ABIs now.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 8, 2016

Hear ye, hear ye! This RFC is now entering final comment period.

I should note that, personally, I'm inclined not to accept, based on @eddyb's comment.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 22, 2016

After discussion in the @rust-lang/lang meeting today, we've decided not to accept this RFC. There doesn't seem to be any particular semantic concerns about allowing methods to have their own ABI, and the implementation is even working as far as we know, so retroactively feature gating is not necessary. Thanks @Aatch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.