-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Do not discard ?Sized
type params and suggest their removal
#87421
Conversation
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 5a7faf8ad5d496c74ac0dd5701ddf4c5f6462f38 with merge 013ac38e83baa419dc36f15d68c4c6bc6e79c1e0... |
☀️ Try build successful - checks-actions |
Queued 013ac38e83baa419dc36f15d68c4c6bc6e79c1e0 with parent 3b4a0df, future comparison URL. |
Finished benchmarking try commit (013ac38e83baa419dc36f15d68c4c6bc6e79c1e0): comparison url. Summary: This benchmark run did not return any significant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. @bors rollup=never |
?Sized
type params and suggest their removal?Sized
type params and suggest their removal
@bors r+ |
📌 Commit 15a40c7 has been approved by |
☀️ Test successful - checks-actions |
@@ -442,6 +442,7 @@ pub enum GenericBound<'hir> { | |||
Trait(PolyTraitRef<'hir>, TraitBoundModifier), | |||
// FIXME(davidtwco): Introduce `PolyTraitRef::LangItem` | |||
LangItemTrait(LangItem, Span, HirId, &'hir GenericArgs<'hir>), | |||
Unsized(Span), |
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 didn't have time to get to this. But I'm a bit concerned about this. Would like to have potentially seen this reuse Trait
in some 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.
I brought this up in #85799 (comment) but then forgot about it again entirely. Sorry, should've paid more attention. I'm fine doing a quick revert and figuring all this out in a new PR
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.
As you all prefer, but I am concerned about the size effects of adding a Span
to Trait
. @oli-obk, that comment was in a separate enum
, to carry the span in the obligation, this is to carry it in the HIR
.
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.
PolyTraitRef
already contains a span, can we re-use that?
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.
Looking back at this, the reason I didn't want to just use the Trait
variant were because we want this exclusively for this, as an "empty signal" and wanted to make sure we didn't try to evaluate these by accident (as they are already handled differently elsewhere). I could have added a field to Trait
, but I thought that would have caused the enum's size to increase (although that might not be the case, looking at the LangItem
variant).
No description provided.