Simplify checking for dependency cycles by Eh2406 · Pull Request #14089 · rust-lang/cargo (original) (raw)

What does this PR try to resolve?

In my PubGrub work I had to try to understand the check_cycles post resolution pass. I found the code tricky to understand and doing some unnecessary reallocations. I cleaned up the code to remove the unnecessary data structures so that I could better understand.

How should we test and review this PR?

It's an internal re-factor, and the tests still pass. But this code is not extensively tested by our test suite. In addition I ran my "resolve every crate on crates.io" script against a local check out of this branch. With the old code as check_cycles_old and the PR code as check_cycles_new the following assertion did not hit:

fn check_cycles(resolve: &Resolve) -> CargoResult<()> { let old = check_cycles_old(resolve); let new = check_cycles_new(resolve); assert_eq!(old.is_err(), new.is_err());

old

}

That experiment also demonstrated that this was not a significant performance change in either direction.

Additional information

If you're having questions about this code while you're reviewing, this would be a perfect opportunity for better naming and comments. Please ask questions.