Skip to content

Added protocol to support CVarArg objects that need to be retained #32311

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 4 commits into from
Sep 10, 2020

Conversation

Molanda
Copy link
Contributor

@Molanda Molanda commented Jun 11, 2020

To fix string formatting using %@ and a String argument, the CVarArg protocol needs to be implemented on String. To do this, String requires an NSString to return a CFString which is what is finally used by the formatting.

However, without an autorelease pool (as is the case with most non-Apple platforms), NSString will get destroyed once out of scope of _cVarArgEncoding.

This pull request adds a new protocol to support retaining such objects without requiring an autorelease pool.

Please see Foundation PR #2821 and this post for more details.

Resolves SR-957.

@millenomi
Copy link
Contributor

This is desirable for source compatibility between Darwin Foundation and corelib Foundation.

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

Since this is affecting public entry points, we either need to add availability for Darwin platforms or we need to make the new entries only available on platforms where there is no Objective-C runtime. I suspect the latter is the better choice?

@millenomi
Copy link
Contributor

millenomi commented Aug 21, 2020

@Molanda I would recommend only #ifdefing to platforms without the ObjC runtime.

@Molanda
Copy link
Contributor Author

Molanda commented Aug 22, 2020

Thank you for the comments.

I will make the changes sometime this weekend.

@Molanda
Copy link
Contributor Author

Molanda commented Aug 22, 2020

I have made the recommended changes so the retaining object protocol is only used when the ObjC runtime is not available.

@DougGregor
Copy link
Member

@swift-ci please test

@DougGregor
Copy link
Member

@lorentey have any opinions on this?

@Molanda
Copy link
Contributor Author

Molanda commented Aug 31, 2020

Just checking in to see if anything additional is needed on this one.

@millenomi
Copy link
Contributor

@DougGregor @lorentey any more feedback? This is highly desired for master.

Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

Looks good to me!

It would be desirable not to autorelease on Apple platforms, either. To do that, we'd probably want to augment the retainer array with inline space for a couple object references (to prevent allocations in the common case), and I'm 99% sure we'd also want to remove @inlinable from all builder methods. (Exposing the vararg builder's class members in the ABI was a regrettable step.)

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.

4 participants