Avoid adding drops for types w/ no dtor in MIR construction #30492
Conversation
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
|
I'm not really sure how to write an automated test for this. If someone can point me in the right direction, I'd be happy to add one. |
wesleywiser
force-pushed
the
fix_extra_drops
branch
from
7df55e4 to
9e2d62e
Compare
| /// (Note that this implies that if `ty` has a destructor attached, | ||
| /// then `type_needs_drop` will definitely return `true` for `ty`.) | ||
| pub fn type_needs_drop(&self, ty: Ty<'tcx>) -> bool { | ||
| self.type_needs_drop_given_env(ty, &self.empty_parameter_environment()) |
There was a problem hiding this comment.
Is it really fine to use empty param environment here? I assume this is supposed to hold some sort of substs for generics. It was fine to do pass empty environment in trans, because all types are monomorphized there, but in middle::ty or mir::hair::cx you cannot assume that types will be monomorphized (they usually won’t be).
| @@ -105,11 +105,7 @@ impl<'a,'tcx:'a> Cx<'a, 'tcx> { | |||
|
|
|||
| pub fn needs_drop(&mut self, ty: Ty<'tcx>, span: Span) -> bool { | |||
| if self.infcx.type_moves_by_default(ty, span) { | |||
There was a problem hiding this comment.
Is this first if redundant now? Maybe the whole method body can be simplified to a single function call?
|
Seems fine apart from the potential redundancy noted above. r=me once that is either removed or explained (in the latter case, a comment on this PR would suffice) |
|
|
wesleywiser
force-pushed
the
fix_extra_drops
branch
from
29bcc99 to
8cc7809
Compare
wesleywiser
force-pushed
the
fix_extra_drops
branch
from
8cc7809 to
8c897ee
Compare
|
💔 Test failed - auto-linux-64-opt |
|
@bors retry |
|
⚡ Previous build results for auto-linux-64-debug-opt, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-64-nopt-t, auto-win-msvc-32-opt are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-opt, auto-win-msvc-64-opt... |
|
Thanks everybody! On Wed, Jan 6, 2016, 2:29 AM bors notifications@github.com wrote:
|
Fixes #28159