-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Cleanup metadata and incremental cache processing of constants #49079
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
I'll review this. |
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.
Thanks, @oli-obk! Looking good!
r=me with the nits addressed.
src/librustc/mir/interpret/mod.rs
Outdated
} | ||
if let Some(alloc) = tcx.interpret_interner.get_alloc(alloc_id) { | ||
trace!("encoding {:?} with {:#?}", alloc_id, alloc); | ||
0usize.encode(encoder)?; |
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.
It would be nice to define constants for those.
src/librustc/mir/interpret/mod.rs
Outdated
>( | ||
decoder: &mut D, | ||
tcx: TyCtxt<'a, 'tcx, 'tcx>, | ||
pos: POS, |
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.
Why not just pass a usize
?
match this.interpret_alloc_shorthands.entry(alloc_id) { | ||
Entry::Occupied(entry) => Some(entry.get().clone()), | ||
Entry::Vacant(entry) => { | ||
assert!(pos != 0 && pos != 1); |
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 add a comment that explains what this assertion is about?
src/librustc_metadata/encoder.rs
Outdated
match this.interpret_alloc_shorthands.entry(alloc_id) { | ||
Entry::Occupied(entry) => Some(entry.get().clone()), | ||
Entry::Vacant(entry) => { | ||
assert!(pos != 0 && pos != 1); |
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.
Same as above.
I decided to fix #49081 here, too, because it's hard/annoying to to before this PR and if I made a second PR they'd just break each other anyway. |
needs a re-review |
@bors: p=1 (I've seen a number of projects that would benefit from getting this quickly!) |
📌 Commit 49dac83 has been approved by |
☀️ Test successful - status-appveyor, status-travis |
fixes #49033
fixes #49081
we really need tests for this. do we have any cross compilation tests? I couldn't find any