Use a graph the represend the dependencies between exported items #44
Conversation
petgraph::Graphs EdgeIndices are not stable over removing (see https://docs.rs/petgraph/0.4.5/petgraph/graph/struct.Graph.html#method.remove_edge), so we should not remove edges by a list of id's.
weiznich
force-pushed
the
feature/dependency_graph
branch
from
4362362
to
f7a9aea
Compare
|
@eqrion I've rebased this pull request again. I would be quite happy if we could work on merging this soon, because it changes a relative central part of cbindgen. Therefore each rebase is quite challenging. |
|
@weiznich Okay, thank you for rebasing. Unfortunately, I'm away for the next week on vacation. I tried to review this before hand but was unable to. I'll review this before making any future changes once I get back, so you won't have to rebase. |
There was a problem hiding this comment.
Just finished looking over this from a high level view, and I think there needs to be a few changes before we could land this.
This commit changes the output headers to allow functions, structs, enums, and template specializations to be intermixed. Whereas before we'd put all the functions at the bottom and all the enums at the top and all the template specializations after the extern "C" block.
We should maintain the existing behavior, because the header files generated with this change are quite confusing. I also don't see any reason we can't maintain this behavior.
Additionally, this commit changes the implementation of monomorphs and of specialization's and I don't see a need for it. I may have missed something.
These changes, if still desired, would be better in a separate pull request.
In the end it is required that they are intermixed because one of the items could depend on other items. (At least as soon as you start to implement the member functions feature). If this is really required it should be possible to retain a similar output order at least as long as there are no dependencies.
As far as I understood the code and the output monomorphs are used for generating the template instantiations( I think it is quite hard to split this in a separate pull request because most parts of this are related to the item graph. Without the graph it would be quite hard to remove the monomorph item. Maybe the specializations item could be removed on it's one and be readded in this pull request. |
Yes, but member functions hasn't been implemented yet so I'd prefer not to have a bunch of code churn before it's needed. This change can be done later in the future if needed.
Yes, that was what I was referring to, you changed the implementation here and I don't think it was needed for this PR. The dependency graph is generated in |
I did not implemented a sorting thing, because I did take this code straight from my member functions implementation. This will be a follow up pull request as soon as this one is merged.
In theory your are right, this is not needed, but this simplifies many things. If we generate all monomorphs first and build then the dependency graph ofter those monomorphed items we will need to visit all items at least twice while building the graph. If we generate the monomorphs while building the dependency graph we only need to visit each item once (when we are adding it to the graph). This also simplifies the handling of the generated template specializations. I'm currently abroad so I've not really any time to work on this for the next two weeks or so. |
|
I've pushed an update that tries to reproduce the old order of items in the generated header files.
|
|
Any news on this? (If not I'm in favour of closing this and using my own fork 😞) |
|
Hey, I'm sorry but I don't think I'll be able to merge this soon. I've had to make other changes for new features to support webrender. It may be best for you to use your own fork for this, for now. |
I think it should be possible to rebase this on the recent changes without any bigger issues. I'm willing to do this if there is a clear path to merge this. Ideally we would first talk about remaining issues in this implementation and finally rebase the then hopefully fixed implementation to master. If you could spare some time to work on this I would be happy to resolve the remaining issues. Otherwise I completely understand that the support for webrenderer required features has much bigger priority and will continue using my own fork. |
|
My apologies for not coming back to this soon enough. As the code has changed a lot since this branch, and I think there are some issues remaining with the desired output for this feature, I'd like to close this and move any discussion to #47. |
No worries. Just ping me as soon as there is some spare time from your side to work again on this.
As far as I remember the output was similar to the original one. The only changes are:
|
See #42 for details.
Fixes #43