Skip to content

Small cleanup for named lazy member loading #29659

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
merged 2 commits into from
Feb 13, 2020

Conversation

slavapestov
Copy link
Contributor

Now that we never fail out of the fast path, we can simplify some logic in the surrounding code. Follow-up to #26975.

@slavapestov slavapestov requested a review from CodaFi February 5, 2020 20:13
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

This cleanup exposed a problem with deserialization recovery and
property wrappers. If deserializing a property backed by a wrapper
failed, the lazy member lookup would fail, but subsequently a
loadAllMembers() call would still load the property.

This behavior is actually incorrect, because silently dropping a
stored property of a @Frozen struct can result in miscompiles.
I've filed rdar://59403542 and rdar://59403617 to track fixing this.

In the meantime, I've tweaked the logic a bit to preserve the old
behavior.
This allows us to simplify lookupDirect() a fair bit as well.
@slavapestov
Copy link
Contributor Author

@xymus Do you mind taking a look at the serialization side of this PR?

@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

Copy link
Contributor

@xymus xymus left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup in serialization, I'll try to get to that FIXME soon.

@slavapestov slavapestov merged commit e37dba1 into swiftlang:master Feb 13, 2020
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.

2 participants