-
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
misc cleanups from debugging something #122392
Conversation
this will also change some existing ice messages, right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just fn instantiate_canonical
and change the first line of its doc comment to mention both placeholders and infer vars?
The function name as is feels a bit too long to me
bcbd564
to
b622bb5
Compare
@matthiaskrgr yes this could change ICE output. I'm not sure how widespread tbh, I can't remember if we ever made it so that ICE output that debug prints Ty's will actually debug print them or not. Regardless I have no idea how we would find all the ICEs out of the thousands of issues that contain old region debug output and update it. Not going to block this PR on that as that is a huge burden for simply changing printing used for debugging rustc. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
r=me after blessing mir opt tests and CI is green |
@bors r=lcnr |
misc cleanups from debugging something rename `instantiate_canonical_with_fresh_inference_vars` to `instantiate_canonical` the substs for the canonical are not solely infer vars as that would be wildly wrong and it is rather confusing to see this method called and think that the entire canonicalization setup is completely broken when it is not 👍 also update region debug printing to be more like the custom impls for Ty/Const, right now regions in debug output are horribly verbose and make it incredibly hard to read but with this atleast boundvars and placeholders when debugging the new solver do not take up excessive amounts of space. r? `@lcnr`
☀️ Test successful - checks-actions |
Finished benchmarking commit (a385e56): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 669.309s -> 667.729s (-0.24%) |
rename
instantiate_canonical_with_fresh_inference_vars
toinstantiate_canonical
the substs for the canonical are not solely infer vars as that would be wildly wrong and it is rather confusing to see this method called and think that the entire canonicalization setup is completely broken when it is not 👍also update region debug printing to be more like the custom impls for Ty/Const, right now regions in debug output are horribly verbose and make it incredibly hard to read but with this atleast boundvars and placeholders when debugging the new solver do not take up excessive amounts of space.
r? @lcnr