Skip to content

[sil] Add clarity to how load_weak [take] and store_weak effect an ob… #21985

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

gottesmm
Copy link
Contributor

…jects weak ref count.

@gottesmm gottesmm requested review from jckarter and rjmccall January 18, 2019 21:17
@gottesmm gottesmm force-pushed the pr-44903c025f4dedd6fcf95759f6512365cd8eb662 branch from 20c0fae to cbb3f11 Compare January 18, 2019 21:19
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test

docs/SIL.rst Outdated
* The value that was originally referenced by the weak reference will have
its weak reference count decremented by 1.
* If the optional is non-nil, the reference in the optional's strong reference count is not
affected.
Copy link
Contributor

@rjmccall rjmccall Jan 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "if" clause is unfortunate here; it ends up suggesting that the reference count is affected if the optional is nil, which is of course not what you mean. Just "The new value's weak reference count is incremented, and its strong reference count is not affected." should be clear enough. If you want to be explicit about what happens in the nil case, you should do that for all the places you're modifying.

Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WFM

@gottesmm
Copy link
Contributor Author

@swift-ci smoke test and merge

3 similar comments
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test and merge

@gottesmm
Copy link
Contributor Author

@swift-ci smoke test and merge

@gottesmm
Copy link
Contributor Author

@swift-ci smoke test and merge

@swift-ci swift-ci merged commit 10f84f0 into swiftlang:master Jan 18, 2019
@gottesmm gottesmm deleted the pr-44903c025f4dedd6fcf95759f6512365cd8eb662 branch April 11, 2019 00:16
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