-
Notifications
You must be signed in to change notification settings - Fork 13.3k
pretty: fix to print some lifetimes on HIR pretty-print #103080
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
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
compiler/rustc_hir_pretty/src/lib.rs
Outdated
if !is_elided_lifetime { | ||
nonelided_generic_args = true; | ||
} | ||
is_elided_lifetime |
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.
A bit messy around here. I feel like if any better way.
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.
Could you replace this branch by changing the if nonelided_generic_args
to if nonelided_generic_args || !elide_lifetimes
below the loop?
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.
Yeah, that would work fine. But I have a concern that it would make nonelided_generic_args
differ from what is inferred from its name.
Because in that case, nonelided_generic_args
can be false, even when we have lifetime generic_args that are not elided.
Isn't it a concern that much? (or do I understand wrong?)
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.
nonelided_generic_args
is only used in one place, just below, so I'm not really concerned. If you want to fix that, you can change the name.
Another possibility: make lt.is_elided()
into an arm guard, and let the non-elided case fall through the _ =>
case.
I'll let this to your judgement.
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.
nonelided_generic_args is only used in one place, just below, so I'm not really concerned.
Ah, indeed.
Another possibility: make lt.is_elided() into an arm guard, and let the non-elided case fall through the _ => case.
Thanks for the suggestion! I prefer this one a bit. But looks like non-elided case cannot fall to the _ =>
case, so may be like this:
GenericArg::Lifetime(lt) if lt.is_elided() => true,
GenericArg::Lifetime(_) => {
nonelided_generic_args = true;
false
}
_ => {
nonelided_generic_args = true;
true
}
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.
done.
106bbbe
to
7334526
Compare
Thanks! |
…imes, r=cjgillot pretty: fix to print some lifetimes on HIR pretty-print HIR pretty-printer doesn't seem to print some lifetimes in types. This PR fixes that. Closes rust-lang#85089
…imes, r=cjgillot pretty: fix to print some lifetimes on HIR pretty-print HIR pretty-printer doesn't seem to print some lifetimes in types. This PR fixes that. Closes rust-lang#85089
…imes, r=cjgillot pretty: fix to print some lifetimes on HIR pretty-print HIR pretty-printer doesn't seem to print some lifetimes in types. This PR fixes that. Closes rust-lang#85089
Rollup of 6 pull requests Successful merges: - rust-lang#101717 (Add documentation about the memory layout of `UnsafeCell<T>`) - rust-lang#102023 (Add MaybeUninit array transpose From impls) - rust-lang#103033 (Update pkg-config) - rust-lang#103080 (pretty: fix to print some lifetimes on HIR pretty-print) - rust-lang#103082 (Surround type with backticks) - rust-lang#103088 (Fix settings page) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
HIR pretty-printer doesn't seem to print some lifetimes in types. This PR fixes that.
Closes #85089