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

Make some NullablePointer enums FFI-compatible with the base pointer type. #14121

Merged
merged 3 commits into from
May 18, 2014

Conversation

luqmana
Copy link
Member

@luqmana luqmana commented May 12, 2014

This slightly adjusts the NullablePointer representation for some enums in the case where the non-nullable variant has a single field (the ptr field) to be just that, the pointer. This is in contrast to the current behaviour where we'd wrap that single pointer in a LLVM struct.

Fixes #11040 & #11303.

@brson
Copy link
Contributor

brson commented May 12, 2014

This may be a bigger problem that calls for a more structured solution. In particular, I think we are going to have this exact same problem again with Box being a struct, not a pointer. I wonder what @nikomatsakis's thoughts on this are.

@nikomatsakis
Copy link
Contributor

r+ modulo nits.

@jld
Copy link
Contributor

jld commented May 14, 2014

I've looked over the patch but haven't reviewed it in detail; it looks reasonable, although it would be nice if there were a way to avoid copying st.fields.len() == 1 into so many places. f+

Also, it's been long enough since I touched this that I don't remember what state the FFI-safe-type lint pass is in with respect to allowing Option<&T> and similar without allowing other non-#[repr(C)] enums, but that might be worth looking at.

@luqmana
Copy link
Member Author

luqmana commented May 14, 2014

@jld I thought about adding another case to Repr but decided against it since there would only be a little different compared to the NullablePointer case.

@nikomatsakis
Copy link
Contributor

@luqmana actually, I think I might prefer to see another case: StructWrappedNullablePointer vs RawNullablePointer or something like that. One consequence is that you could not fail to forget a case. Given that the merge failed, I'd like to change my r+ to r- and request that change. :)

@nikomatsakis
Copy link
Contributor

@luqmana any idea what's the deal with those Travis CI failures?

@nikomatsakis
Copy link
Contributor

Giving r+ in any case, I guess we'll see if those are real or not.

bors added a commit that referenced this pull request May 18, 2014
This slightly adjusts the NullablePointer representation for some enums in the case where the non-nullable variant has a single field (the ptr field) to be just that, the pointer. This is in contrast to the current behaviour where we'd wrap that single pointer in a LLVM struct.

Fixes #11040 & #11303.
@bors bors closed this May 18, 2014
@bors bors merged commit be79edb into rust-lang:master May 18, 2014
@luqmana luqmana deleted the option-ffi branch May 23, 2014 18:28
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.

C func returning Option<extern fn> is not immediate on 32-bit linux
6 participants