Issue #5977 - Generalizing RawNullablePointer to RawForbiddenValue #31215
Conversation
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
Yoric
force-pushed
the
forbidden
branch
2 times, most recently
from
2a1317c
to
33b2674
Compare
| assert_eq!(ix, 0); | ||
| val | ||
| RawForbiddenValue { payload_discr, .. } => { | ||
| if _discr == payload_discr { |
There was a problem hiding this comment.
You can remove the _ here since the variable is now used.
|
\o/ seems good! Paves the way for more cool optimizations. |
|
I haven't managed to generalize |
brson
added
the
relnotes
|
@bors r+ |
|
📌 Commit 962221b has been approved by |
Yoric
force-pushed
the
forbidden
branch
3 times, most recently
from
8ab79b1
to
6dec707
Compare
|
Fixed style and a test that was accidentally ripped from my next patch on the line. |
|
☔ The latest upstream changes (presumably #30448) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Fixed. |
| }, | ||
| ty::TyRawPtr(..) | ty::TyInt(..) | ty::TyUint(..) => { | ||
| path.push(0); | ||
| Some(path) | ||
| Some((path, C_null(type_of::sizing_type_of(cx, ty)))) |
There was a problem hiding this comment.
ty here would be the NonZero strict so I don't think that's what you want.
There was a problem hiding this comment.
Not sure I understand your sentence, but I just realized that ty was shadowed and that I should probably use the shadowed version instead. Is this the problem you were mentioning?
There was a problem hiding this comment.
s/strict/struct/ :P
So, the forbidden value will be compared against the discriminant field. Say we had Option<NonZero<int>>, we're testing that that nested int is non-zero. Currently, it's returning C_null of NonZero (the passed in ty to the function) as the forbidden value rather than of the int field (field_ty).
There was a problem hiding this comment.
Got it, thanks. I obviously misunderstood that block.
|
r? @pnkfelix |
|
@luqmana I can put a |
…r> and Option<bool> r?luqmana
|
/me grumbles something about 100-chars limit. |
|
☔ The latest upstream changes (presumably #31417) made this pull request unmergeable. Please resolve the merge conflicts. |
| @@ -438,23 +469,34 @@ struct Case<'tcx> { | |||
| /// This represents the (GEP) indices to follow to get to the discriminant field | |||
| pub type DiscrField = Vec<usize>; | |||
|
|
|||
| fn find_discr_field_candidate<'tcx>(tcx: &ty::ctxt<'tcx>, | |||
| fn find_discr_field_candidate<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, | |||
There was a problem hiding this comment.
Please either add a doc comment explaining what the two parts of the returned tuple are and/or switch from using a tuple to using a struct with self-explanatory named fields
|
commits lgtm: |
| @@ -1 +1 @@ | |||
| Subproject commit 30f70baa6cc1ba3ddebb55b988fafbad0c0cc810 | |||
| Subproject commit 91ff43c736de664f8d3cd351e148c09cdea6731e | |||
There was a problem hiding this comment.
hmm was this an accidental commit?
|
@Yoric I'd say this is definitely on the right track! Great stuff, sorry it took me so long to get around to looking seriously at it. If you rebase I suspect I'll just do a superficial review before r-plussing. (In other words, if another reviewer happens to get to it and wants to do r=pnkfelix, that will probably be fine.) |
|
Closing due to inactivity, but feel free to resubmit with a rebase! |
This is a WIP first step towards Issue #5977. Could someone (@luqman?) take a first look and tell me whether I'm heading in the right direction?