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

Fix order of structs #42

Closed

Conversation

weiznich
Copy link
Contributor

Improved version of #39
Fixed Example:

struct List<T> {
     members: *mut T,
     count: usize
}

struct A;

struct B;

extern "C" fn foo(a: List<A>) {}
extern "C" fn bar(b: List<B>) {}

Before this commit the following code was generated:

struct A {};

struct List_A {
    A* members;
    size_t count;
};

struct List_B {
    B* members;
    size_t count;
}

struct B {};

After this commit the order of the generated structs are fixed:

struct A {};

struct B {};

struct List_A {
    A* members;
    size_t count;
};

struct List_B {
    B* members;
    size_t count;
}

@eqrion
Copy link
Collaborator

eqrion commented Aug 12, 2017

A major refactoring of cbindgen just landed and I think this issue has been resolved. Can you test and let me know if this is still an issue?

@weiznich
Copy link
Contributor Author

weiznich commented Aug 14, 2017

A major refactoring of cbindgen just landed and I think this issue has been resolved. Can you test and let me know if this is still an issue?

The Example from the pull request seems to be fixed now, but this does not resolve the general underlying issue. For example the following snippet will generate an invalid header:

#[repr(C)]
pub struct Foo {
    a: i32,
    b: *const Bar,
}

#[repr(C)]
pub struct Bar {
    a: *mut Foo,
}

#[no_mangle]
pub extern "C" fn foo(f: Foo) {}

generates the following header:

extern "C" {

struct Bar {
  Foo *a;
};

struct Foo {
  int32_t a;
  const Bar *b;
};

void foo(Foo f);

} // extern "C"

As soon as #36 lands the problem does get even more complex, because then we could not even output first the structs and then the generated extern functions, because to generate the member functions we will need to have access to each of the argument types and the inlined function. Each argument type could be a complete class on each own.

Let me try to outline the underlying problem: Each item could depend on a set on other items. At the time of collecting all function arguments one could not determine on which position an item should be printed. The current implementation tries to do the collection and the ordering in one step.

A possible solution for this problem seems to be to collect everything into a graph. Each node represents a specific item (e.g. a Struct or a function) and each edge represents the kind of the dependency between two items (Is it a pointer or a normal use?). For the example above the following graph is generated:
full

After that one could traverse the graph and find nodes without any dependency. This node could be added to the output and then be removed from the graph. This step is repeated till the graph is empty or there is no node without dependency left. (For example this is the case for the example above, because there is a circular dependency.). Now one will need to resolve the cycle. Because the graph contains information about which kind of dependency is used between two items it is now possible to search for pointeronly dependencies. To resolve those dependencies an opaque item could be injected and the corresponding edge can be removed. This will break the cycle and one could repeat the normal algorithm to determine the next output element.
After breaking the cycle the graph for the example above looks like:
removed_cycle
Now one could remove each node step by step, because they do not depend on each other anymore:
removed_1_item.
This results in the following generated code:

struct Foo;

struct Bar {
  Foo *a;
};

struct Foo {
  int32_t a;
  const Bar *b;
};

void foo(Foo f);

If implemented all this on the following branch in my repo to see how it interacts with the member function feature. (Unfortunately it is all smashed together in one branch and not rebased on master 😢 )

(Sidenote: The implementation in this pull request is quite broken and should better not be used… If you are open to implement something like this, we should better try to merge the referenced branch.)

@eqrion
Copy link
Collaborator

eqrion commented Aug 16, 2017

Yes, I've been considering creating an explicit graph for tracking dependencies for some time. Mostly for #12 and #11. I hadn't really considered keeping track of two types of edges (ptr and normal), but that would definitely allow us to generate forward declarations for structs when needed.

I think that would definitely be worthwhile, I think it could be implemented by rewriting the 'Dependencies' class to create an explicit graph, then creating an ordering of definitions and forward declarations using it.

I'm open to a PR for this, or I will implement it in a couple of weeks when I get time.

@weiznich
Copy link
Contributor Author

I'm open to a PR for this, or I will implement it in a couple of weeks when I get time.

I will try to rework my branch to have a pull request for this, but this could take some days(Let's see when there is some time…)

I think implementing the dependency graph should be done before starting to implement member functions, because it is the much bigger change (in terms of complexity and code size).

After that implementing member function should be easy to add. The "only" pain point there is to figure out the exact format of the generated header. (Although this should also be mostly done. At least for our use case it is now working (Calling a destructor, calling member functions, returning complex types…). )

@eqrion
Copy link
Collaborator

eqrion commented Aug 17, 2017

I've filed #43 as an issue for this problem. Would you mind closoing this PR and opening up a clean one once you have an implementation?

@djpiper28
Copy link

I am having errors from this and I am going to fix it and call it exam revision. (if I don't fail at fixing it lmao)

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.

3 participants