fix(engine/deps): preserve definitions order whenever possible #1151
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
W95Psp
force-pushed
the
fix-item-order-711-transitive-closure
branch
2 times, most recently
from
1ed3e2a to
7041ef6
Compare
|
@W95Psp please reference the issue that this is closing |
In most of the backends, a top-level definition should come prior to it's use. This is not true in Rust. Thus, we need to sort the items so that item `a` comes before it is used in another item `b`. We used to do that with a topological sort with the ocamlgraph library. But since this topological sort operated on a graph, we were loosing the original order of items. This commit now: - construct a the transtive closure of the dependency graph - use this closure to stable sort the items The comparison function is defined as follows (`g` denotes the transitive closure): 1. if there is an edge `a ⟶ b` in `g` and an edge `b ⟶ a`, we are in a recursive bundle, hence we return `0`: `a` equals `b` 2. if there is an edge `a ⟶ b` in `g`, we return `-1` 2. if there is an edge `b ⟶ a` in `g`, we return `1` 2. otherwise there is no relation between `a` and `b`, we return `0`
W95Psp
force-pushed
the
fix-item-order-711-transitive-closure
branch
from
7041ef6 to
aa33ea4
Compare
|
I hit this issue again yesterday when working on Bertie. Things were reordered with every change I made to some modules, for no good reason. I believe it also makes it hard to diff and debug the effect of engine changes (since the order change would make the diffs look much bigger than they are). So I encourage converting this into a reviewable PR whenever some other work item needs it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In most of the backends, a top-level definition should come prior to it's use. This is not true in Rust.
Thus, we need to sort the items so that item
acomes before it is used in another itemb.We used to do that with a topological sort with the ocamlgraph library. But since this topological sort operated on a graph, we were loosing the original order of items.
This commit now:
The comparison function is defined as follows (
gdenotes the transitive closure):a ⟶ bingand an edgeb ⟶ a, we are in a recursive bundle, hence we return0:aequalsba ⟶ bing, we return-1b ⟶ aing, we return1aandb, we return0The run for this PR on libcrux is there: