Skip to content
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

[IRGen] Do not write out ObjC property ivar field for static properties #21435

Merged
merged 2 commits into from Dec 21, 2018

Conversation

fay59
Copy link
Contributor

@fay59 fay59 commented Dec 19, 2018

This PR ensures that Swift does not fill the ivar attribute of Objective-C property metadata for class properties, since class properties are never backed by an ivar.

Resolves SR-9541.

I expect that my computer will be building Swift for a good amount of time, so I'll change the tests and run them when I'm back from work, or some time later depending on my availability. Just leaving this out there since it's a super simple change and I assigned the bug to myself.

rdar://problem/46830117

@jrose-apple
Copy link
Contributor

Thanks, Félix!

@jrose-apple jrose-apple self-requested a review December 19, 2018 18:34
@slavapestov
Copy link
Member

@zneak Generally a change like this needs a test case. A good place to add one might be test/IRGen/objc_properties.swift. You can find a brief introduction to FileCheck in docs/Testing.md -- let us know if you need help. That document also tells you how to run the entire test suite or individual tests.

@fay59
Copy link
Contributor Author

fay59 commented Dec 20, 2018

@slavapestov, thanks for the help! objc_properties (and its _ios sibling) is the file that I identified this morning before work. My Swift build was still in progress and I didn't want to commit tests before getting a chance to run them. I'm flying tomorrow and I have to pack, but hopefully I'll have something shortly.

@fay59
Copy link
Contributor Author

fay59 commented Dec 20, 2018

Test has been updated, I think that this is ready for review.

@jrose-apple
Copy link
Contributor

I spotted another bug in this implementation, which is that I'm not sure it's correct to print the ivar name if the storage doesn't match the property (for example, NSString vs. Swift.String). But that's a separate issue.

@swift-ci Please test

@jrose-apple jrose-apple self-assigned this Dec 20, 2018
@swift-ci

This comment has been minimized.

@swift-ci

This comment has been minimized.

@fay59
Copy link
Contributor Author

fay59 commented Dec 20, 2018

You’re right, this is also an issue. Is there a bug for it?

@jrose-apple
Copy link
Contributor

I don't think I've seen one.

@jrose-apple
Copy link
Contributor

Unrelated LLDB failures. Let's try again.

@swift-ci Please test

@jrose-apple jrose-apple merged commit 3844dfd into apple:master Dec 21, 2018
@jrose-apple
Copy link
Contributor

Thanks, Félix! I'll cherry-pick this to the 5.0 branch for you too.

@fay59 fay59 deleted the objc-static-ivars branch December 21, 2018 02:12
@fay59
Copy link
Contributor Author

fay59 commented Dec 21, 2018

Great! For your trouble, I'll file a bug for this other related issue.

@fay59
Copy link
Contributor Author

fay59 commented Dec 21, 2018

I filed it as SR-9557. I'm not sure that I'll pick it up, though.

jrose-apple added a commit to jrose-apple/swift that referenced this pull request Dec 21, 2018
[IRGen] Do not write out ObjC property ivar field for static properties
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.

None yet

4 participants