-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
refactor destructure_const
#150411
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
refactor destructure_const
#150411
Conversation
|
|
This comment has been minimized.
This comment has been minimized.
|
btw dont worry about the commit history- I expect this PR is simple enough that just one is fine. thanks for splitting it up nicely though :-) |
3f89fa8 to
88a8d43
Compare
This comment has been minimized.
This comment has been minimized.
88a8d43 to
e3605ec
Compare
|
oh no i forget about semicolon |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
seems like it also fixes #131052? |
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.
Can you squash all these commits then undraft the PR? I think its basically done after these reviews. I just want to spend a bit of time thinking over why this fixed that ICE but I don't expect that to take me long
yeah, this commits are only for while it's draft |
c49d62f to
41ef5c7
Compare
|
hopefully i didnt mess with commits |
41ef5c7 to
3085d3d
Compare
This comment has been minimized.
This comment has been minimized.
| } | ||
| (ty::ValTreeKind::Branch(fields), ty::Adt(def, args)) => { | ||
| let contents = cv.destructure_adt_const(); | ||
| let fields = fields.iter().copied(); |
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.
using fields here is wrong as it has more stuff in it than contents' fields
3085d3d to
46b2837
Compare
|
This PR changes a file inside Some changes occurred in cc @BoxyUwU |
|
pls squash commits and look at the two leftover reviews, thx |
46b2837 to
12e8fd0
Compare
This comment has been minimized.
This comment has been minimized.
12e8fd0 to
0c53563
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
0c53563 to
b29cc98
Compare
| @@ -0,0 +1,13 @@ | |||
| //! Regression test for <https://github.com/rust-lang/rust/issues/131052> | |||
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 think what's happened here is that we're now masking an ICE that was originally caused by lit_to_const returning ValTree's with incorrect types 🤔 I can't think of a way to write a test that exposes that underlying brokenness so I think this is fine.
It would be nice to fix the underlying brokenness regardless. Would you be interested in doing that? The short explanation is that we lit_to_const for byte strings doesn't check that the type of the parameter the byte string is an argument to, is &[u8] is only checks if its a reference to a slice.
|
@bors r+ rollup thanks for the PR ✨ |
|
i also was wondering, during this i've seen a lot of FIXME(mgca) and some of them seemed pretty solvable, at least at a first glance, would this be reasonable to try to solve them, or it'd be better to ask you first? |
yeah i guess i could take a look potentially, around next few days maybe |
some of them are probably a lot harder than others. if you ask me first I can tell you whether it's likely to be a huge pain or relatively simple :) in general though help with mgca is much appreciated |
refactor `destructure_const` r? BoxyUwU as you suggested yesterday (will add more context like links when it stops being a draft) tried to split this into some meaningful commits hope it help ^^
…uwer Rollup of 8 pull requests Successful merges: - #148321 (parser/lexer: bump to Unicode 17, use faster unicode-ident) - #149540 (std: sys: fs: uefi: Implement readdir) - #149582 (Implement `Duration::div_duration_{floor,ceil}`) - #149663 (Optimized implementation for uN::{gather,scatter}_bits) - #149667 (Fix ICE by rejecting const blocks in patterns during AST lowering (closes #148138)) - #149947 (add several older crashtests) - #150011 (Add more `unbounded_sh[lr]` examples) - #150411 (refactor `destructure_const`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #150411 - Kivooeo:somefunnystuff, r=BoxyUwU refactor `destructure_const` r? BoxyUwU as you suggested yesterday (will add more context like links when it stops being a draft) tried to split this into some meaningful commits hope it help ^^
r? BoxyUwU
as you suggested yesterday (will add more context like links when it stops being a draft)
tried to split this into some meaningful commits hope it help ^^