Skip to content
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

Change method of duplicate string in NSString.CacheContainer #139

Merged
merged 2 commits into from
Jan 17, 2016

Conversation

norio-nomura
Copy link
Collaborator

@@ -54,8 +54,7 @@ extension NSString {
//
// A reference to `NSString` is held by every cast `String` along with their views and
// indices.
let cString = string.cStringUsingEncoding(NSUTF8StringEncoding)
let string = String(CString: cString, encoding: NSUTF8StringEncoding)!
let string = string.mutableCopy() as! String
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this mutableCopy() and not just copy()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NSString.copy returns self because that is immutable. NSString.mutableCopy returns new instance of NSString with copied buffer. On this case, the string holds reference of the CacheContainer through associated object.
On baa8a3a, ByteOffsetCache(renamed to CacheContainer) held reference of string by holding String.UTF8Index and that caused circular reference.
On 6656224, it resolved by changing to create new instance.
Using mutableCopy() replaces that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

NSString(string: string) as String ?

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the detailed explanation!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@segiddins
Apple's document said:

  • initWithString:
    Returns an NSString object initialized by copying the characters from another given string.

But, you can confirm that is incorrect as following:

import Foundation

let str1 = "Hello, playground" as NSString
let str2 = NSString(string: str1)
let str3 = str1.mutableCopy() as! NSString

let id1 = ObjectIdentifier(str1)
let id2 = ObjectIdentifier(str2)
let id3 = ObjectIdentifier(str3)

id1 == id2 // true
id1 == id3 // false

@jpsim
Copy link
Owner

jpsim commented Jan 16, 2016

Thanks for the fix! I'm just not sure why it has to be a mutable copy.

jpsim added a commit that referenced this pull request Jan 17, 2016
Change method of duplicate string in `NSString.CacheContainer`
@jpsim jpsim merged commit 2deab89 into jpsim:master Jan 17, 2016
@norio-nomura norio-nomura deleted the nn-fix-swiftlint-379 branch January 17, 2016 02:18
@norio-nomura
Copy link
Collaborator Author

🙏

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