test: add fixes in the sat resolver #14707
Conversation
rustbot
added
A-dependency-resolution
|
|
||
| for &feature_name in dep.features() { | ||
| if let Entry::Vacant(entry) = dep_feature_vars.entry(feature_name) { | ||
| entry.insert(solver.new_var()); |
There was a problem hiding this comment.
Why the change? Is this just to avoid allocating several new_var's? Do we really end up with dependencies that activate the same feature more than once?
Also would this be clearer as .entry(feature_name).or_insert_with(|| solver.new_var())?
There was a problem hiding this comment.
Why the change? Is this just to avoid allocating several
new_var's?
Yes
Also would this be clearer as
.entry(feature_name).or_insert_with(|| solver.new_var())?
I didn't use or_insert_with because I don't need the returned Entry.
There was a problem hiding this comment.
Okay, that makes sense.
Do we really end up with dependencies that activate the same feature more than once?
For testing purposes I added a else {todo!()} to each of these, and all tests pass. So I don't think our test suite hits this case. What am I missing? Do you have plans that will add test code that hits this case? Are there significant downsides to a few calls to solver.new_var()? I'm not objecting, I'm just trying to understand.
There was a problem hiding this comment.
The else {todo!()} branch will trigger if we have a dependency with a list of features containing duplicates, or if the feature "dep/feature" or "dep?/feature" is listed multiple times among all features declared in a summary.
I will add a test for this.
There was a problem hiding this comment.
Are there significant downsides to a few calls to solver.new_var()?
Maybe not, but removing duplicates will improve the debugging if we want to manually inspect the clauses.
|
No need for a test. @bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Update cargo 14 commits in cf53cc54bb593b5ec3dc2be4b1702f50c36d24d5..e75214ea4936d2f2c909a71a1237042cc0e14b07 2024-10-18 13:56:15 +0000 to 2024-10-25 16:34:32 +0000 - refactor(env): remove unnecessary clones (rust-lang/cargo#14730) - test(install): Verify 2024 edition / resolver=3 doesn't affect resolution (rust-lang/cargo#14724) - Fix: trace `config` `[env]` table in dep-info. (rust-lang/cargo#14701) - Added unstable-schema generation for Cargo.toml (rust-lang/cargo#14683) - fix: add source replacement info when no matching package found (rust-lang/cargo#14715) - feat(complete): Include descriptions in zsh (rust-lang/cargo#14726) - refactor(fingerprint): avoid unnecessary fopen calls (rust-lang/cargo#14728) - docs(resolver): Make room for v3 resolver (rust-lang/cargo#14725) - test: add fixes in the sat resolver (rust-lang/cargo#14707) - docs(ci): Don't constrainty latest_deps job by MSRV (rust-lang/cargo#14711) - refactor: use `Iterator::is_sorted` (rust-lang/cargo#14702) - refactor(rustfix): minor refactors (rust-lang/cargo#14710) - chore(deps): update msrv (rust-lang/cargo#14705) - fix(renovate): Switch matchPackageNames to matchDepNames (rust-lang/cargo#14704)
What does this PR try to resolve?
This is a follow-up of #14614.
How should we test and review this PR?
Commit 1 removes duplicate variables in the sat resolver.
Commit 2 removes useless clones in the sat resolver.
r? Eh2406