Forward declared structs now generate opaque enums #370

nikhilshagri · 2016-12-29T12:59:38Z

@emilio : I checked the test outputs again, and it seems that these changes are affecting struct definitions as well. Hence, I have not committed the test changes yet.
Fixes #62

emilio · 2016-12-29T13:04:16Z

libbindgen/src/ir/comp.rs

@@ -236,6 +236,8 @@ pub struct CompInfo {
    /// Used to detect if we've run in a has_destructor cycle while cycling
    /// around the template arguments.
    detect_has_destructor_cycle: Cell<bool>,
+
+    is_declaration: bool,


let's rename this and the method to is_forward_declaration, and also add a doc comment here describing what this indicates.

emilio · 2016-12-29T13:06:27Z

libbindgen/src/ir/comp.rs

@@ -506,6 +509,7 @@ impl CompInfo {
        debug!("CompInfo::from_ty({:?}, {:?})", kind, cursor);

        let mut ci = CompInfo::new(kind);
+        ci.is_declaration = cursor.definition().unwrap().is_declaration();


This is maybe not enough? The definition is also a declaration, we need to search for a declaration that has no definition.

Note that clang has CXCursor_NoDeclFound, so we can probably do something like:

ci.is_forward_declaration = cursor.definition().map_or(true, |d| d.kind() == CXCursor_NoDeclFound);

That might be enough.

Why should the default value be true here?

what if we modify the from_ty function to accept a cursor as well? We could simple use clang_isDeclaration() then.

It's only true if the definition is invalid.

nikhilshagri · 2016-12-30T14:11:12Z

Is there any way I can print out the cursor kind and run transformations on a single header file? This might give us some more information.

emilio · 2016-12-30T15:28:55Z

You can run bindgen on a header file with the --emit-clang-ast option, and also with RUST_LOG=libbindgen to see pretty much everything you could need.

…

On Fri, Dec 30, 2016 at 06:11:12AM -0800, Nikhil Shagrithaya wrote: Is there any way I can print out the cursor kind and run transformations on a single header file? This might give us some more information. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #370 (comment)

nikhilshagri · 2016-12-30T21:00:35Z

cursor.defintion() always returns a cursor of type StructDecl, it doesn't matter whether the cursor points to a struct definition or a declaration.

One solution, like I suggested earlier, is to modify the CompInfo::from_ty method to accept a cursor as well, and use that cursor to determine whether it points to a declaration or not.

nikhilshagri · 2017-01-05T19:55:23Z

@emilio I got two suggestions from twitter, which work somewhat:

Use cursor.clang_isCursorDefinition(), but this doesn't work for templated structs. No false positives here.
Parse the cursor as a token using clang_tokenize() and check if the name ends with a semicolon.

Option 2 is a hack, but it could be useful. Should I try using a combination of these two to see if it works?

emilio · 2017-01-06T07:50:35Z

clang_isCursorDefinition sounds great, I'd rather patch libclang than tokenize, so lets use that :)

emilio · 2017-01-06T07:51:51Z

BTW, clang_isCursorDefinition was mentioned in the issue, I assumed you'd tried it :)

nikhilshagri · 2017-01-06T19:43:54Z

I'd tried it initally for the cursor we obtain from ty.declaration() in from_ty, but forgot to test it for the one in from_clang_ty.

nikhilshagri · 2017-01-16T14:36:50Z

libbindgen/tests/expectations/tests/namespace.rs

@@ -66,6 +66,7 @@ pub mod root {
            #[link_name = "_ZN1w3fooEv"]
            pub fn foo() -> root::C<::std::os::raw::c_int>;
        }
+        pub enum C { }


I don't know why this is appearing here, even though the struct has already been declared a few lines above.

nikhilshagri · 2017-01-16T14:41:59Z

@emilio I've updated the PR, but like I said, there's still a few false negatives that I can't get around. For example, template < typename > struct char_traits; does not resolve into an enum, because the corresponding CursorKind is ClassTemplate.

emilio

Looks way better! :)

emilio · 2017-01-16T15:09:44Z

libbindgen/tests/expectations/tests/anon_union.rs

-    pub _address: u8,
-    pub _phantom_0: ::std::marker::PhantomData<T>,
-}
+pub enum TErrorResult_Message { }


This is wrong, you still need to make it take a template parameter, there is no way this is compiling otherwise.

emilio · 2017-01-16T15:11:22Z

libbindgen/src/codegen/mod.rs

@@ -723,6 +723,17 @@ impl CodeGenerator for CompInfo {
            return;
        }

+        // generate an opaque enum if struct is a forward declaration
+        if self.kind() == CompKind::Struct && self.is_forward_declaration() {


Given that, let's skip also structs with template parameters.

Why only check for Struct? instead of both struct and union?

emilio · 2017-01-16T15:13:11Z

libbindgen/src/clang.rs

@@ -190,6 +190,10 @@ impl Cursor {
        unsafe { clang_getCursorKind(self.x) }
    }

+    pub fn is_definition(&self) -> bool {
+        unsafe { clang_isCursorDefinition(self.x) == 0 }


This is returning true if the cursor is not a definition, which causes a lot of confusion. Can you change this check to != 0, and invert the conditional it in other places?

Also, this is going to need a line of documentation or this won't pass CI.

emilio · 2017-01-16T15:14:40Z

libbindgen/src/ir/comp.rs

@@ -506,6 +512,8 @@ impl CompInfo {
        debug!("CompInfo::from_ty({:?}, {:?})", kind, cursor);

        let mut ci = CompInfo::new(kind);
+        ci.is_forward_declaration = location.map_or(true, |cur|
+                                        cur.is_definition() && cur.kind() == CXCursor_StructDecl);


Why only if the cursor is CXCursor_StructDecl?

Also, I think this should work ok if you just do:

ci.is_forward_declaration = !cursor.is_definition();

That should make it work for structs with template parameters too. Does that work?

Alternatively, worth trying !cursor.definition().is_valid().

bors-servo · 2017-01-23T09:38:01Z

☔ The latest upstream changes (presumably #414) made this pull request unmergeable. Please resolve the merge conflicts.

nikhilshagri · 2017-01-23T14:43:09Z

What happened here??!! :P

fitzgen · 2017-01-23T16:11:09Z

What happened here??!! :P

:) Everything that was in ./libbindgen/* should now be in ./* for the most part :P

briansmith · 2017-01-23T20:27:30Z

See also rust-lang/rfcs#1861 and the referenced internals discussion about why using empty enums for this is (probably) wrong.

fitzgen · 2017-01-23T21:03:13Z

See also rust-lang/rfcs#1861 and the referenced internals discussion about why using empty enums for this is (probably) wrong.

I'm not 100% sure that argument applies since we would be manipulating pointers to such enums, not the enum directly.

nikhilshagri · 2017-01-24T22:26:12Z

@briansmith We decided to go ahead with this and if needed, change it later :)

nikhilshagri · 2017-01-24T22:27:26Z

src/ir/comp.rs

@@ -481,6 +486,9 @@ impl CompInfo {
        debug!("CompInfo::from_ty({:?}, {:?})", kind, cursor);

        let mut ci = CompInfo::new(kind);
+        ci.is_forward_declaration = location.map_or(true, |cur|
+                                        (cur.kind() == CXCursor_StructDecl || cur.kind() == CXCursor_UnionDecl) &&


@emilio It might be a good idea to add a test for unions as well, since none of the existing tests are testing it.

briansmith · 2017-01-24T22:31:29Z

@briansmith We decided to go ahead with this and if needed, change it later :)

If rust-bindgen starts using the empty enum pattern then it will create a lot of inertia preventing any deprecation of that pattern, which would be bad. Why not use the similar thing that the Rust lang team recommends instead, according to rust-lang/rfcs#1861 (comment):

#[repr(C)]
pub struct Opaque([u8; 0]);

emilio · 2017-01-24T23:11:24Z

That sounds fine to me Brian. @cynicaldevil, it shouldn't be hard to change, instead of EnumBuilder you can use the tuple_struct method from aster, or the quote_item! macro, at your choice.

There are examples of both in that same file.

Thanks for commenting here @briansmith, I was the one initially proposing opaque enums but I wasn't aware of the issues.

Whether those issues are more or less likely to happen in practice for bindgen doesn't seem too relevant if there's a better and equally simple to implement alternative.

Thanks again!

nikhilshagri · 2017-01-25T00:31:11Z

@emilio pushed new changes.

emilio · 2017-01-25T10:26:14Z

I'm surprised this only hits once, may you add a few more tests for this? The common patters of forward-declaring a struct or union, then defining functions taking a pointer to that should be enough.

nikhilshagri · 2017-01-26T11:36:55Z

How about this:

struct Foo;

struct Bar {
    Foo *f;
};

void baz_struct(Foo* f);

union Union;

void baz_union(Union* u);

// conversion to tuple struct does not work on classes
class Quux;

emilio · 2017-01-26T11:44:34Z

That looks quite fine to me. We should be able to make it work on classes without template parameters though. But we can leave that as a followup if you want.

nikhilshagri · 2017-01-26T12:04:27Z

I added a condition to generate the tuple struct for classes as well.

emilio

The rest looks great! Thanks for doing this :)

emilio · 2017-01-26T12:39:19Z

src/ir/comp.rs

@@ -481,6 +486,10 @@ impl CompInfo {
        debug!("CompInfo::from_ty({:?}, {:?})", kind, cursor);

        let mut ci = CompInfo::new(kind);
+        ci.is_forward_declaration = location.map_or(true, |cur|


we could write this as:

ci.is_forward_declaration = location.map_or(true, |cur| { match cur.kind() { CXCursor_StructDecl | CXCursor_UnionDecl | CXCursor_ClassDecl => !cur.is_definition(), _ => false, } });

Which I think it's a bit more legible, what do you think?

emilio · 2017-01-26T12:40:04Z

tests/headers/forward_declared_complex_types.hpp

+struct Bar {
+    Foo *f;
+};
+


Could you also add an empty struct (struct Baz {}), and verify that it doesn't produce an opaque enum?

emilio · 2017-01-26T13:11:36Z

@bors-servo r+

Thanks for doing this @cynicaldevil \o/

bors-servo · 2017-01-26T13:11:48Z

📌 Commit 78aaa32 has been approved by emilio

bors-servo · 2017-01-26T13:11:54Z

⌛ Testing commit 78aaa32 with merge 5cc2506...

@emilio

Forward declared structs now generate opaque enums @emilio : I checked the test outputs again, and it seems that these changes are affecting struct *definitions* as well. Hence, I have not committed the test changes yet. Fixes #62

bors-servo · 2017-01-26T13:23:12Z

☀️ Test successful - status-travis

highfive added the S-awaiting-review label Dec 29, 2016

emilio reviewed Dec 29, 2016

View reviewed changes

nikhilshagri commented Jan 16, 2017

View reviewed changes

emilio requested changes Jan 16, 2017

View reviewed changes

highfive added the S-needs-rebase label Jan 23, 2017

nikhilshagri force-pushed the detect-forward branch from 1716936 to c4ade05 Compare January 24, 2017 22:23

nikhilshagri commented Jan 24, 2017

View reviewed changes

nikhilshagri force-pushed the detect-forward branch from c4ade05 to 4e0a624 Compare January 25, 2017 00:29

nikhilshagri force-pushed the detect-forward branch from 4e0a624 to 9866229 Compare January 26, 2017 12:03

emilio approved these changes Jan 26, 2017

View reviewed changes

nikhilshagri added 2 commits January 26, 2017 18:27

Forward declared structs now generate opaque enums

8eb9e3f

Added test for forward declared complex types

78aaa32

nikhilshagri force-pushed the detect-forward branch from 9866229 to 78aaa32 Compare January 26, 2017 13:05

highfive assigned emilio Jan 26, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Jan 26, 2017

bors-servo merged commit 78aaa32 into rust-lang:master Jan 26, 2017

highfive removed the S-awaiting-merge label Jan 26, 2017

Forward declared structs now generate opaque enums #370

Forward declared structs now generate opaque enums #370

Conversation

nikhilshagri commented Dec 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhilshagri commented Dec 30, 2016

emilio commented Dec 30, 2016 via email

nikhilshagri commented Dec 30, 2016

nikhilshagri commented Jan 5, 2017 • edited Loading

emilio commented Jan 6, 2017

emilio commented Jan 6, 2017

nikhilshagri commented Jan 6, 2017

Choose a reason for hiding this comment

nikhilshagri commented Jan 16, 2017

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Jan 23, 2017

nikhilshagri commented Jan 23, 2017

fitzgen commented Jan 23, 2017

briansmith commented Jan 23, 2017

fitzgen commented Jan 23, 2017

nikhilshagri commented Jan 24, 2017

Choose a reason for hiding this comment

briansmith commented Jan 24, 2017

emilio commented Jan 24, 2017

nikhilshagri commented Jan 25, 2017

emilio commented Jan 25, 2017

nikhilshagri commented Jan 26, 2017

emilio commented Jan 26, 2017

nikhilshagri commented Jan 26, 2017

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio commented Jan 26, 2017

bors-servo commented Jan 26, 2017

bors-servo commented Jan 26, 2017

bors-servo commented Jan 26, 2017

nikhilshagri commented Dec 29, 2016 •

edited

Loading

nikhilshagri commented Jan 5, 2017 •

edited

Loading