Ensure we do not zero when "moving" types that are Copy #22818
Conversation
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
|
(caveat: i have not yet run this through even a cc @nikomatsakis ; I'll happily take a review from anyone who wants to give it, but @eddyb is certainly appropriate too. |
|
see also #22815 -- I had meant to audit (and write comments attached to) all the calls to |
|
(also, I definitely want to play around and write some more tests related to the corner cases here. For example, there are notes in the commits regarding the behavior of the |
It passes |
|
(one might reasonably point out that I should have alpha-renamed the old ((one might also reasonably point out that better fixes to #22815 would actually make this particular PR unnecessary.)) |
|
@pnkfelix what about |
|
|
||
| /// Issue #22815: call the `FunctionContext::type_needs_drop` method | ||
| /// instead, if possible; it is strictly more precise than this | ||
| /// function. | ||
| pub fn type_needs_drop<'tcx>(cx: &ty::ctxt<'tcx>, ty: Ty<'tcx>) -> bool { |
There was a problem hiding this comment.
I'd prefer to remove this fn, or make it scoped to FunctionContext::type_needs_drop -- is there a reason we can't do this?
There was a problem hiding this comment.
I guess this was only implicit in the PR: the two cases where I left calls to common::type_needs_drop were because there is not a FunctionContext readily available. I spent a while tracing back through the call chain to see if I could refactor the code to pass down a Block (with its attached FunctionContext) instead of passing down a CrateContext in each of the two cases, but in both of them as I worked my way backwards it seemed not feasible. (In particular, in one case the call chain goes back to translating const expressions, where I assume it is by design that there is no FunctionContext available.)
There was a problem hiding this comment.
(okay, fixed so that one can call either one; they have the same semantics now.)
|
@nikomatsakis has pointed out that even an empty |
…e::zero_mem`. (Instrumentation work for Issue 22536.)
pnkfelix
force-pushed
the
fsk-issue-22536
branch
from
5c261bd to
9a6e3b9
Compare
|
⌛ Testing commit 9a6e3b9 with merge e4498c1... |
|
💔 Test failed - auto-win-32-nopt-t |
|
@alexcrichton this looks safe to retry, yes? (I will be heading off to bed now, so if you could hit "retry" in my stead I would greatly appreciate it!) |
|
@bors: retry |
|
⌛ Testing commit 9a6e3b9 with merge 431430c... |
|
💔 Test failed - auto-mac-64-nopt-t |
…sakis Ensure we do not zero when \"moving\" types that are Copy. Uses more precise `type_needs_drop` for deciding about emitting cleanup code. Added notes about the weaknesses regarding `ty::type_contents` here. Fix rust-lang#22536
|
@bors: retry |
Ensure we do not zero when "moving" types that are Copy.
Uses more precise
type_needs_dropfor deciding about emitting cleanup code. Added notes about the weaknesses regardingty::type_contentshere.Fix #22536