Refactor context to extract dependencies calculation to a separate mod#5232
Refactor context to extract dependencies calculation to a separate mod#5232bors merged 1 commit intorust-lang:masterfrom
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
alexcrichton
left a comment
There was a problem hiding this comment.
An awesome refactoring, thanks!
There was a problem hiding this comment.
I think this may not need to be public, right?
There was a problem hiding this comment.
I might find this a bit clearer as:
match deps.entry(*unit) {
Entry::Occupied(v) => v.into_ref(), // or whatever the method is
Entry::Vacant(v) => v.insert(calculate_deps(...)),
}There was a problem hiding this comment.
I may be missing something here, though? Is it required to insert into the map before we do the recursive step? I would have figured that the recursion would have been ruled out by this point...
There was a problem hiding this comment.
.entry would not work here, because we need mutable access to deps in the call to calculate_deps :(
The recursion here is needed only for side effects: we want to fill dependencies not only for root units, but for the whole transitive closure. Ideally, this function should have returned (), but we actually call it recursively when we figure out deps for custom_build.
There was a problem hiding this comment.
Hm ok I'm still not 100% following but that's fine, can clean it up as need be in the future
There was a problem hiding this comment.
Is this not possible due to various methods that take &mut self on Context?
There was a problem hiding this comment.
Yep! Specifically, this happens because target_filenames and target_metadatas are calculated lazily. I hope to refactor them out as well, and change a whole bunch of methods from &mut to &.
There was a problem hiding this comment.
This is here because build_map requires dep_targets, right?
|
@bors: r+ |
|
📌 Commit 1b63fcf has been approved by |
Refactor context to extract dependencies calculation to a separate mod This makes unit dependency graph construction eager and moves it to a separate file. Hopefully, this makes `Context` slightly easier to understand :)
|
☀️ Test successful - status-appveyor, status-travis |
This makes unit dependency graph construction eager and moves it to a separate file.
Hopefully, this makes
Contextslightly easier to understand :)