-
Notifications
You must be signed in to change notification settings - Fork 556
Elaborate on dataflow outputs for region constraints #1969
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
Conversation
...hapter Splitting the two was confusing and meant that similar information was in wildly different parts of the guide. Combine them into a single page.
mostly this just suggests using a tracking issue instead of inlining the info from the issue template
I'm going to make a follow-up PR shortly linking from rust-lang/rust to the dev guide so this info isn't duplicated.
Co-authored-by: Yuki Okushi <jtitor@2k36.org>
rust-lang#1960) * compiletest: specify which special env var and which particular CI job * compiletest: fix grammar and add link to Dockerfile
Signed-off-by: needsure <qinzhipeng@outlook.com>
The `-Z verbose` option has been renamed to `-Z verbose-internals` in commit b5d83619 [1] (PR #119129 [2]). This commit updates the remaining `-Z verbose` to `-Z verbose-internals`. [1]: rust-lang/rust@b5d8361 [2]: rust-lang/rust#119129
Add a hint about unflatten and an internal link to MIR outlives graphs to advertise them better.
I see Zed snuck in some Markdown normalisation too while it was at it. Sorry about that, let me know if that's inexcusable and I will revert those changes.
I will revert those changes
Please do yes, if it's not too hard. It will make review easier.
I'll take a closer look when I have more time, but I noted a couple of things:
- "With
-Z dump-mir-graphviz=yes, you will also get Graphviz files for the outlives constraints" is that the case? Did you maybe mean-Zdump-mir=nll? - I don't think these visualizations are from MIR dataflow, so the new paragraph may need to be moved to somewhere more related to borrowck
1dcef2a to
2c521d5
Compare
Please do yes, if it's not too hard. It will make review easier.
Done!
"With
-Z dump-mir-graphviz=yes, you will also get Graphviz files for the outlives constraints" is that the case? Did you maybe mean-Zdump-mir=nll?
Huh! Apparently, -Z dump-mir=fn is enough to get those. I assumed it wouldn't drop graphviz files without the Graphviz option, but apparently it does! dump-mir=nll also works and now I'm worried it will dump different graphs.
I don't think these visualizations are from MIR dataflow, so the new paragraph may need to be moved to somewhere more related to borrowck
That's probably true; this should be in the borrowck chapter, maybe? Do you have a suggestion off the top of your head or should I go digging?
Not from the top of my head but I will look for one -- but yeah I agree this should be in the borrowck chapter most likely.
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.
(Sorry for taking so long to get back to this.)
So since these are not dataflow outputs, I agree we should add it to the borrow_check chapter.
To do so, I think we should add a dedicated borrow_check/debugging.md page, and have the "Region constraint graphs and their SCCs" section there. We would mention that these come from -Zdump-mir=nll -- and that will give us the occasion to also describe the NLL dumps there in the future.
We can link to that page from the bottom of src/borrow_check.md instead of src/compiler-debugging.md (but we can leave the unflatten paragraph there, it's a good addition next to the graphviz section).
Co-authored-by: Rémy Rakic <remy.rakic+github@gmail.com>
Co-authored-by: Rémy Rakic <remy.rakic+github@gmail.com>
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.
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.
This line will need to be wrapped as well.
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.
@lqd btw, you can also edit @amandasystems's branch directly for trivial things like fixing links. We can also try to get this merged so it doesn't drown from merge conflicts. And it's perfectly fine if it still has inaccuracies/omissions, we can always follow-up, it's just docs after all!
Sorry, due to me messing up a git operation, we sadly had to force-push the whole commit history of rustc-dev-guide :( If you'd like to update this pull request, you will have to rebase it in a special way onto the new commit history (the new master):
git fetch origin --all
git checkout <pr-branch>
git rebase --onto origin/master origin/master-old
git push --force-with-lease
More context can be found here.
No description provided.