Skip to content

Conversation

gottesmm
Copy link
Contributor

…and do not conditionalize based off of the ownership flag being set.

This is the last part of SILGen conditionalized on EnableSILOwnership being
set. It also (as you can tell from the diff) eliminates a bunch of code from the
tests.

rdar://29791263

…and do not conditionalize based off of the ownership flag being set.

This is the last part of SILGen conditionalized on EnableSILOwnership being
set. It also (as you can tell from the diff) eliminates a bunch of code from the
tests.

rdar://29791263
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

@swift-ci test source compatibility

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 489910e

@gottesmm
Copy link
Contributor Author

java errors...

@gottesmm
Copy link
Contributor Author

@swift-ci text os x platform

@gottesmm
Copy link
Contributor Author

@swift-ci test source compatibility

@gottesmm
Copy link
Contributor Author

@swift-ci test os x platform

@atrick
Copy link
Contributor

atrick commented Oct 24, 2018

I don't understand why coroutines would be handled differently than normal applies. A coroutine is itself a "scoped" operation". So it uses it's operands up to the end_apply. There's no reason to add another borrow scope around the coroutine.

@atrick
Copy link
Contributor

atrick commented Oct 24, 2018

That said, it look like these are mostly test case changes, and it would be easy to remove the isForCoroutine flag later... as long as the overall end design is that begin_apply/apply have the same ownership rules, but any code that determines lifetime based on apply arguments needs to also find the end_apply.

@gottesmm
Copy link
Contributor Author

I talked with Andy offline. I am fine with having begin_apply/end_apply be like begin/end borrow for these uses. I do think it complicates the model slightly (since this is another formed of scoped use), but you could also argue the other way that considering apply and begin_apply as different things also complicates the model. Andy and I agreed that we will try this and if it confuses people down the line, we can put in the begin_borrow scope for coroutine. I am going to make that change in a subsequent commit though.

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 489910e

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 489910e

@gottesmm
Copy link
Contributor Author

@swift-ci smoke test

@gottesmm gottesmm merged commit 965d697 into swiftlang:master Oct 25, 2018
@gottesmm gottesmm deleted the pr-c8913074c7375798bf6ea153a3635e28ec248f06 branch October 25, 2018 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants