Introduce Requirements struct to clarify code #4683
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
@alexcrichton this is what I started with at RustFest. I have some more work done on using features as a type, but that requires yet more untangling of all the related code passing around features as strings. |
|
|
||
| fn require_dependency(&mut self, pkg: &'r str) { | ||
| if self.seen(pkg) { | ||
| return; |
There was a problem hiding this comment.
I think this early return may cause the boolean in the deps entry to be false when it should unconditionally be true if this function is called, right?
There was a problem hiding this comment.
That's also what happens in the old version, right? I tried to keep it functionally the same; reviewing it now, this seems to be the behavior of the old code, too.
There was a problem hiding this comment.
(Also, wouldn't you say that the boolean in deps must already be true if pkg is self.seen() here?)
There was a problem hiding this comment.
Ah ok I think I see that now, makes sense! I find this code always pretty hard to follow...
I completely forget at this point why these all exist as they are, this has changed a fair bit since first written.
| } | ||
|
|
||
| fn require_crate_feature(&mut self, package: &'r str, feat: &'r str) { | ||
| self.used.insert(package); |
There was a problem hiding this comment.
I'd naively expect this to call require_dependency and then fill in the deps map list, but that also seems like the kind of thing that may break tests...
There was a problem hiding this comment.
I'm still a little fuzzy on what used means exactly, and how it's different from the deps boolean being true. This is different at least in that it leaves the deps boolean at false, which seems meaningful because setting a "forwarded" feature does not mean that we should actually pull that dependency in.
There was a problem hiding this comment.
I think it may be related to #4364? I think that changed a few things in this area
|
@bors: r+ |
|
📌 Commit abf4122 has been approved by |
Introduce Requirements struct to clarify code `cargo::core::resolver::build_requirements()` previously, in this order: * Defined local variables to track state * Called a function to mutate the local variables * Defined the aforementioned function * Returned two out of three local variables as a tuple This PR changes the code so that this is a recast as a struct (appropriately called `Requirements`), which is mutated in a more fine-grained way by its methods and acts as the return type for `build_requirements()`. To me, this seems a lot easier to understand. This work was done in the context of #1286, but I figured it was easier to start landing some of the refactoring to avoid bitrot and get early (well, earlier) feedback.
|
☀️ Test successful - status-appveyor, status-travis |
cargo::core::resolver::build_requirements()previously, in this order:This PR changes the code so that this is a recast as a struct (appropriately called
Requirements), which is mutated in a more fine-grained way by its methods and acts as the return type forbuild_requirements(). To me, this seems a lot easier to understand.This work was done in the context of #1286, but I figured it was easier to start landing some of the refactoring to avoid bitrot and get early (well, earlier) feedback.