[Runtime] Add a comment to explain the previous change. #16197
Conversation
The reason for have BridgeObjectBox::retain() not return the value from swift_bridgeObjectRetain is not obvious.
|
@swift-ci please smoke test |
| @@ -411,6 +411,8 @@ struct BridgeObjectBox : | |||
|
|
|||
| static void *retain(void *obj) { | |||
| (void)swift_bridgeObjectRetain(obj); | |||
| // The return value from swift_bridgeObjectRetain may not include the | |||
There was a problem hiding this comment.
Would it make more sense to change swift_bridgeObjectRetain() instead?
There was a problem hiding this comment.
I wondered the same thing, but I have no idea what would be involved in that.
There was a problem hiding this comment.
Yes that makes sense. I did some git spelunking. The function originally (at some point in history) was:
void *swift::swift_bridgeObjectRetain(void *object) {
#if SWIFT_OBJC_INTEROP
if (isObjCTaggedPointer(object))
return object;
#endif
auto const objectRef = toPlainObject_unTagged_bridgeObject(object);
#if SWIFT_OBJC_INTEROP
if (!isNonNative_unTagged_bridgeObject(object))
return swift_retain(static_cast<HeapObject *>(objectRef));
return objc_retain(static_cast<id>(objectRef));
#else
return swift_retain(static_cast<HeapObject *>(objectRef));
#endif
}
So my concern when I proposed the fix to BridgeObjectBox that somebody relies on the different return value is probably not justified.
There was a problem hiding this comment.
Ah nevermind, that previous code would not justify the change -- it is also returning the untagged object.
In any case, I do not believe anyone (else) was using the return value.
| @@ -411,6 +411,8 @@ struct BridgeObjectBox : | |||
|
|
|||
| static void *retain(void *obj) { | |||
| (void)swift_bridgeObjectRetain(obj); | |||
| // The return value from swift_bridgeObjectRetain may not include the | |||
There was a problem hiding this comment.
Yes that makes sense. I did some git spelunking. The function originally (at some point in history) was:
void *swift::swift_bridgeObjectRetain(void *object) {
#if SWIFT_OBJC_INTEROP
if (isObjCTaggedPointer(object))
return object;
#endif
auto const objectRef = toPlainObject_unTagged_bridgeObject(object);
#if SWIFT_OBJC_INTEROP
if (!isNonNative_unTagged_bridgeObject(object))
return swift_retain(static_cast<HeapObject *>(objectRef));
return objc_retain(static_cast<id>(objectRef));
#else
return swift_retain(static_cast<HeapObject *>(objectRef));
#endif
}
So my concern when I proposed the fix to BridgeObjectBox that somebody relies on the different return value is probably not justified.
|
In theory this should be all that is needed: #16213 |
|
I have merged #16213. Assuming it sticks. This commit is no longer necessary. Closing. |
|
Awesome! Thanks, Arnold |
The reason for have BridgeObjectBox::retain() not return the value
from swift_bridgeObjectRetain is not obvious.