Skip to content

Conversation

jrose-apple
Copy link
Contributor

SR-8356 / rdar://problem/42284266

@jrose-apple
Copy link
Contributor Author

@swift-ci Please test

@jrose-apple
Copy link
Contributor Author

@swift-ci Please test source compatibility

@@ -437,7 +437,8 @@ class ObjCProtocolInitializerVisitor
Builder.CreateCall(protocol_addMethodDescription, getterArgs);

if (prop->isSettable(nullptr)) {
emitObjCSetterDescriptorParts(IGM, prop, name, types, imp);
emitObjCSetterDescriptorParts(IGM, prop, /*extended*/true,
name, types, imp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there rhyme or reason for why some of these use extended encodings and others don't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Protocols do, non-protocols don't. I don't know the story further than that, though; I just imitated what was already there. (That's why I tagged Greg for review, really.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I guess that makes as much sense as anything. Could you make the fact that that's the reason behind the decision more explicit in the source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I don't think this is correct after all. There's already a separate path for getting extended type encodings for protocols. @gparker42, how is this supposed to work?

These are only used in two places in the Apple Objective-C runtime:

- A protocol's "extended method types" list
- Block type descriptors

We were using them when dynamically registering a protocol with the
Objective-C runtime, but that's just expecting "normal" types; the
"extended method types" list is never present in such a protocol.
@jrose-apple
Copy link
Contributor Author

Okay, I investigated where it's correct to use extended types, and apparently it's not in at least one of the places where we were. Fixed that and added a test.

@rjmccall, can you re-review?

@swift-ci Please test

@jrose-apple
Copy link
Contributor Author

@swift-ci Please test source compatibility

@swift-ci
Copy link
Contributor

swift-ci commented Dec 9, 2018

Build failed
Swift Test OS X Platform
Git Sha - e93fa16b703f43bee2fc0bcc6de38189d263065c

@jrose-apple
Copy link
Contributor Author

*sigh* ObjCBool…

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.

This seems reasonable to me. What's going on with ObjCBool?

@jrose-apple
Copy link
Contributor Author

Oh, just that the test case I modified was already limited to CPU=x86_64 because whoever wrote it originally didn't feel like conditionalizing it, but I forgot that that includes the 64-bit simulator. I think it's probably okay to just change it to OS=macosx, since it wasn't going to be testing most platforms anyway.

We visited them twice, which led to registering them twice. Add a test
for this feature so that we don't regress on this or on the use of
non-extended types in the future (see previous commits).
@jrose-apple
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Dec 9, 2018

Build failed
Swift Test Linux Platform
Git Sha - e93fa16b703f43bee2fc0bcc6de38189d263065c

@swift-ci
Copy link
Contributor

swift-ci commented Dec 9, 2018

Build failed
Swift Test OS X Platform
Git Sha - e93fa16b703f43bee2fc0bcc6de38189d263065c

@jrose-apple jrose-apple merged commit b060166 into swiftlang:master Dec 10, 2018
@jrose-apple jrose-apple deleted the sub-rosa branch December 10, 2018 19:58
jrose-apple added a commit to jrose-apple/swift that referenced this pull request Dec 10, 2018
[IRGen] Generate proper type descriptors for ObjC subscript accessors

https://bugs.swift.org/browse/SR-8356
(cherry picked from commit b060166)
jrose-apple added a commit that referenced this pull request Dec 11, 2018
[IRGen] Generate proper type descriptors for ObjC subscript accessors

https://bugs.swift.org/browse/SR-8356
(cherry picked from commit b060166)
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