Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Better pretty printing for const raw pointers #65859
Better pretty printing for const raw pointers #65859
Changes from all commits
22586d7
6a6ed12
d322044
3364c46
f2cb4d7
6c26bd6
175c608
c2dbe44
27d89ec
1407cdd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
So this is a "fallback" case now, right? We already have a big case for references above. Unfortunately it is not immediately clear what distinguishes these cases.
Could you add some comments both at the
if let ty::Ref
above and at the match arm here describing why we do the matches we do in the order they are, and which various cases of references each conditional covers?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.
I don't know the history of that big
if let ty::Ref
statement or why it looks the way it does internally, so I don't feel comfortable documenting it. But I added a comment specifying that it needs to happen before the general case.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.
From what I can see, the point of that is to print the content of the slice/array when possible for types
&[u8]
,[u8; N]
and&str
. Do you agree? If yes, please write that. Right now you only mention&str
but that is not the only type handled here.The fallback then is that for all other references and raw pointers, we just print the pointer value, not the thing it points to.
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 is weirdly inconsistent between references and raw pointers. To be fair, it was also inconsistent before, but now the inconsistency is stronger.
Also @oli-obk @wesleywiser could you check these
-Zdump-mir
changes. This is a regression in terms of information content but I am not sure if anyone cares.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.
Mir dumps should probably dump the content of referenced allocations, like it dumps promoteds. Without showing alloc IDs this would be impossible to figure out though. I think mir dumps should keep dumping alloc ids
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 is where I got stuck. I still haven't been able to chase down where this Debug print comes from. Maybe we should open another issue?
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.
You are stuck trying to find out where the old printing for references came from, or the new one, or the one for raw pointers?
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.
The one for raw pointers. But with @oli-obk's comment in mind, maybe the opposite fix is more appropriate, replacing the new pretty printed references with a more verbose format? Either way it seems out of scope of this PR to me.
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.
Well the PR changes behavior in an undesirable way when printing references, that part is in scope.
What is being debug-printed here is a
ConstKind
, I think. You could try removing theDebug
impl for that and see where compilation fails.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.
Good idea, thanks!
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.
FWIW, on current nightly this prints
So, yeah, the new output is much better. :D
It seems like right now the code is shared between printing this error and printing the mir-dump, and that is likely not what we want?