Skip to content

[test] Remove StringMemoryTest #59413

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 1 commit into from
Jun 13, 2022
Merged

Conversation

Azoy
Copy link
Contributor

@Azoy Azoy commented Jun 13, 2022

This test is flaky on lots of configs, and its original purpose is no longer valid. String hashing and various other facilities are all native now and don't use Foundation or any of the NSString entry points (thus the autoreleasepool concern doesn't make sense now).

    runtime: Move String implementation stubs that want need the auto-released return value optimization to an ARC compiled file
    
    String's hashValue function is implemented in terms of Foundation's hash
    function in a runtime function on darwin platforms. For non-ASCII strings we
    will call str.decomposedStringWithCanonicalMapping inside this runtime function
    which will allocate a new NSString and return the result in the current
    autorelease pool. We implemented this function in a file compiled without ARC.
    This meant that we would leak said NSString into the current active autorelease
    pool.
    This patch moves the implementation to a file compiled with ARC. ARC will insert
    objc_retainAutoreleasedReturnValue call and on platforms that require it an
    marker for the hand-off of the autoreleased return value optimization.
    
    SR-4889
    rdar://32199117

@Azoy
Copy link
Contributor Author

Azoy commented Jun 13, 2022

@swift-ci please test

@Azoy Azoy merged commit 1891677 into swiftlang:main Jun 13, 2022
@Azoy Azoy deleted the remove-string-memory-test branch June 13, 2022 23:34
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.

2 participants