Skip to content

Add opaque structs to TRPL:FFI #27542

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

Merged
merged 1 commit into from
Aug 11, 2015
Merged

Add opaque structs to TRPL:FFI #27542

merged 1 commit into from
Aug 11, 2015

Conversation

steveklabnik
Copy link
Member

Fixes #27303

@rust-highfive
Copy link
Contributor

r? @nikomatsakis

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

@steveklabnik
Copy link
Member Author

r? @alexcrichton

@geofft
Copy link
Contributor

geofft commented Aug 5, 2015

It would be nice if you mentioned the C headers/prototypes that are being bound here, e.g.

struct Foo; /* Foo is a structure, but its contents are not part of the public interface */
struct Bar;
void foo(struct Foo *arg);
void bar(struct Bar *arg);

so that people know what an opaque struct looks like in C. This is something you don't really have to pay attention to a library user, so the terminology might be unclear (especially the distinction from the C++ or nonstandard C syntax struct Foo {/* no members */};.)

Which brings up one quibble I have here: if the original C API actually does use opaque structs, instead of void pointers, then C users of that API are type-safe. C compilers will warn you about casts between pointers of different types, unless void * is involved. So the first set of Rust bindings in these docs is less type-safe than C, which is probably not a thing we want to encourage. The phrasing "we can at least get type-safety on the Rust side" is a bit misleading here.

I guess you were envisioning binding a C API that uses void * in its public declarations, but I think that lots of C APIs make good use of pointers to opaque structs for this very reason, and that's the use case that #27303 was getting at. Maybe it's worth mentioning both styles of C prototype, and making it clear that you can do the type-safe thing in Rust, whether or not the original C API is type-safe.

}
```

By using an `enum` with no variants, and by _not_ annotating it with `repr(C)`, we
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to imply that in order to create an opaque type that we can't instantiate, we must not annotate it with repr(C), which is a bit confusing since #[repr(C)] enum Foo {} doesn't compile. Maybe you want something more like

"By using an enum with no variants, we create an opaque type that we can't instantiate. (Note that we don't annotate it with repr(C), which isn't valid syntax for enums.)"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually one of the special cases where #[repr(C)] on an enum is invalid, but there are cases where that's useful:

#[repr(C)]
enum Foo {
    Bar,
    Baz = 3,
    Foobar,
}

I believe you just need to have at least 2 variants and none of the variants can have a data payload.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i'll just take that bit out, as it's more distracting

@steveklabnik
Copy link
Member Author

Try this new version on for size.

```

This is a perfectly valid way of handling the situation. However, we can do a bit
better. To solve this, some C libraries will instead create a `struct`, where
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear what "this" is -- did it get lost in refactoring? Did you want to say that foo and bar take different types of arguments?

@alexcrichton
Copy link
Member

@bors: r+ 8c4dc18 rollup

Manishearth added a commit to Manishearth/rust that referenced this pull request Aug 11, 2015
bors added a commit that referenced this pull request Aug 11, 2015
@bors bors merged commit 8c4dc18 into rust-lang:master Aug 11, 2015
@steveklabnik steveklabnik deleted the gh27303 branch June 19, 2016 20:32
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.

8 participants