-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Still more ObligationForest
improvements.
#64805
Conversation
This is the end of my spate of |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@nnethercote what sort of test failures were you observing from (b) above? Are we talking about injecting unsoundness bugs into compiler? Or changes to diagnostic output? Or something else? |
They were all changes in error messages that arose from changing the order of pending obligations. Some of them looked pretty trivial (e.g. change the order of two errors) but some of them were more complex, to the point where I was struggling to tell if the new messages were as valid as the old ones. And there were quite a few in the latter basket. So I ended up preserving the order because the potential speed benefits of reordering were small. |
The test failures on automation are trivial; some vectors of erroneous obligations have changed order, but (AIUI) the order is not significant. I will fix those on Monday. |
It's not necessary; cycles (which are rare) can be detected by looking at the node stack. This change speeds things up slightly, as well as simplifying the code a little.
For consistency with other variables in this file.
It has a single call site, and the code is easier to read this way.
They don't help performance at all, and just complicate things.
These make the code more concise.
By collecting the done obligations (when necessary) in the main loop. This makes the code cleaner. The commit also changes the order in which successful obligations are returned -- they are now returned in the registered order, rather than reversed. Because this order doesn't actually matter, being only used by tests, the commit uses `sort()` to make the test agnostic w.r.t. the order.
3af2b25
to
a820672
Compare
I fixed the test failures and rebased. Ready for review, @nikomatsakis! |
@bors r+ |
📌 Commit a820672 has been approved by |
… r=nikomatsakis Still more `ObligationForest` improvements. Following on from rust-lang#64627, more readability improvements, but negligible effects on speed. r? @nikomatsakis
Rollup of 11 pull requests Successful merges: - #64649 (Avoid ICE on return outside of fn with literal array) - #64722 (Make all alt builders produce parallel-enabled compilers) - #64801 (Avoid `chain()` in `find_constraint_paths_between_regions()`.) - #64805 (Still more `ObligationForest` improvements.) - #64840 (SelfProfiler API refactoring and part one of event review) - #64885 (use try_fold instead of try_for_each to reduce compile time) - #64942 (Fix clippy warnings) - #64952 (Update cargo.) - #64974 (Fix zebra-striping in generic dataflow visualization) - #64978 (Fully clear `HandlerInner` in `Handler::reset_err_count`) - #64979 (Update books) Failed merges: - #64959 (syntax: improve parameter without type suggestions) r? @ghost
Following on from #64627, more readability improvements, but negligible effects on speed.
r? @nikomatsakis