Do not emit shadow copied for inout parameters #5218
Conversation
…ajrawi/swift into inout_shadow_escaping_closure
|
@atrick Can you please review? |
|
@swift-ci Please smoke test |
There was a problem hiding this comment.
Can you identify some unit tests that handle captured inout, and are you convinced they do the right thing? Can you add a test from the bug report to verify that local functions capturing inout are now handled the same as anonymous closures?
Otherwise this looks fine to me.
| @@ -262,19 +262,10 @@ struct ArgumentInitHelper { | |||
| gen.B.createDebugValueAddr(loc, address, {vd->isLet(), ArgNo}); | |||
| return; | |||
| } | |||
| gen.VarLocs[vd] = SILGenFunction::VarLoc::get(argrv.getValue()); | |||
| assert(argrv.getType().isAddress() && "expected inout to be address"); | |||
| gen.B.createDebugValueAddr(loc, argrv.getValue(), {vd->isLet(), ArgNo}); | |||
There was a problem hiding this comment.
The BuiltinUnsafeValueBufferType case above passes the unmanaged address to DebugValueAddr. This new default case passes a the argv value directly to DebugValueAddr. Why is there any difference?
There was a problem hiding this comment.
Combined debug value creation + added unit tests as requested.
|
Awesome, thank you for working on this Joe, I look forward to this all getting cleaner and simpler. |
…dled the same as anonymous closures
|
@swift-ci Please smoke test |
|
Linux smoke test failing with the usual non-PR related error - rerunning: |
|
@swift-ci Please smoke test |
|
Before this is merged, could you please squash @dduan's commits? |
|
Yes please squash...these were never meant to be public :) |
|
I intended to auto-squash everything into one commit on merge - all commits are about the same thing :) |
|
@shajrawi Have you forgotten to squash the commits? |
|
@swiftix I squashed it - it is a single commit on mainline, no? (I clicked squash and merge In the PR) |
|
@shajrawi Ah, OK. I assumed you'd squash and push --force on the PR first and then merge. This is the usual way to it, because then the PR looks the same as the final merged set of commits. |
radar rdar://problem/28434323
SILGen has no reason to insert shadow copies for inout parameters any more. They cannot be captured. We still emit these copies. Sometimes deshadowing removes them, but sometimes it does not.
In this PR we just avoid emitting the copies and remove the deshadowing pass.
This PR chery-picked some of @dduan work and built on top of it.