Skip to content

[Runtime] Add a comment to explain the previous change. #16197

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[Runtime] Add a comment to explain the previous change. #16197

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions stdlib/public/runtime/MetadataImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make more sense to change swift_bridgeObjectRetain() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wondered the same thing, but I have no idea what would be involved in that.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// tag bits; return the input value instead.
return obj;
}

Expand Down