Resolve the order of dependencies in the clike language backend. #1050
Conversation
flaviojs
force-pushed
the
resolve-order-dependencies-clike
branch
from
d9d9d19 to
f265f9e
Compare
|
Had to remove the code that recognizes auto-declarations, which caused unexpected problems. Ideally, the resolver should be fuzzed to make sure different initial orders are always compilable. |
There was a problem hiding this comment.
So it's been a while I haven't thought about this problem because in general it's very easy to just add forward-declarations are needed.
But it seems requiring another global pass seems rather unfortunate? Rather than that, shouldn't add_dependencies be able to track cycles (and break them by inserting a forward declaration if possible)?
E.g., when add_dependencies finds a pointer, it seems it shouldn't add a "hard" dependency, but a "soft" one, which can be dealt with by a forward declaration.
Then that information can be used to sort the items and add any forward declaration as needed.
|
|
||
| #[test] | ||
| fn test_c() { | ||
| compile(Language::C, CODE); |
There was a problem hiding this comment.
We already compile our tests on automation. So please add regular tests rather than this?
| /// Returns `Err(n)` when it gives up with `n` pending items. | ||
| pub fn resolve(&mut self) -> Result<(), usize> { | ||
| loop { | ||
| if self.pending.is_empty() { |
There was a problem hiding this comment.
while !self.pending.is_empty() {?
| /// Does not try to preserve the original order. | ||
| pub fn new(config: &'a Config, dependencies: &'a mut Dependencies) -> Self { | ||
| let num_items = dependencies.order.len(); | ||
| let mut paths2indexes: MultiMap<Path, usize> = MultiMap::new(); |
There was a problem hiding this comment.
Rather than pulling a new dependency for this, I think we can just use a HashMap<Path, Vec<usize>> right?
It can reorder dependencies and add forward declarations.
Warns if there is stuff that it cannot resolve.
Includes inner-working traces that can be seen with
cbindgen -vv(should be left there to facilitate future changes).Includes tests for assumptions that
ResolveOrderis making.Includes a clippy MSVR change to 1.74 (same as
Cargo.toml) to fix a warning.Fixes #43
Maybe replaces #1028 ?
Before this I had to add extra stuff in
after_includesfor about 10% of symbols that ended up initems. All those are handled, leaving only the cases where reordering items and adding forward declarations cannot fix the problem (like constants used in structs appearing after the items are written).I'm sure there are C/C++ cases that are not being handled, but the documentation and tests should facilitate implementing whatever is left.
I reeeally wanted to do some refactoring to facilitate changes and add documentation before attempting this, but the "single person reviewing and approving" bottleneck prevents that solution. I reached a point where it's impossible to continue developing without adding a hack or implementing something like this.
@emilio if there is a proper way for PRs to signal they are "ready to review" after your initial review is done and more changes are applied, please document it somewhere. =~~