[SR-6139] Implement NSString.copy() method #1291

sashabelonogov · 2017-11-01T22:31:13Z

A potential fix for this issue:
https://bugs.swift.org/browse/SR-6139

Before, the method that is responsible to create the immutable copy of NSString was simply
returning self, that’s why copying didn’t happen and this test was failing:

func test_constructorWithCopyItems() {
        let foo = "foo" as NSMutableString

        let array1 = NSArray(array: [foo], copyItems: false)
        let array2 = NSArray(array: [foo], copyItems: true)

        XCTAssertEqual(array1[0] as! String, "foo")
        XCTAssertEqual(array2[0] as! String, "foo")

        foo.append("1")
        XCTAssertEqual(array1[0] as! String, "foo1")
        // The below test fails on sclf
        XCTAssertEqual(array2[0] as! String, "foo")
    }

Please, let me know what you think.

spevans · 2017-11-01T22:34:07Z

@swift-ci please test

sashabelonogov · 2017-11-01T23:58:14Z

I apologize for a failed build, I rushed with solving merge conflicts in a test and missed one "}" in the end. I fixed it in the same commit. Everything should be fine now.

ianpartridge · 2017-11-02T00:27:24Z

@swift-ci please test

ianpartridge · 2017-11-02T00:28:02Z

@swift-ci please test

spevans · 2017-11-02T09:35:04Z

@sashabelonogov Could you also uncomment the following lines in TestFoundation/TestNSArray.swift

//foo.append("1")
//XCTAssertEqual(array1[0] as! String, "foo1")
//XCTAssertEqual(array2[0] as! String, "foo")

As this was the original failing test case so checking it now works would be good, thanks.

sashabelonogov · 2017-11-02T09:46:02Z

@spevans sure! I also deleted my test because it's identical to the existing one.

spevans · 2017-11-02T09:47:13Z

@swift-ci please test

spevans · 2017-11-02T11:14:22Z

@swift-ci please test

alblue

Minor changes - can you make them, then rebase against master and squash changes before resubmitting?

alblue · 2017-11-02T16:21:20Z

Foundation/NSString.swift

-            throw NSError(domain: NSCocoaErrorDomain, code: CocoaError.fileReadInapplicableStringEncoding.rawValue, userInfo: [
-                "NSDebugDescription" : "Unable to create a string using the specified encoding."
-                ])
+                                                throw NSError(domain: NSCocoaErrorDomain,


Can you revert this reformatting, since it isn't really relevant or part of this change?

alblue · 2017-11-02T16:22:06Z

TestFoundation/TestNSArray.swift

@@ -46,7 +46,7 @@ class TestNSArray : XCTestCase {
            ("test_mutableCopying", test_mutableCopying),
            ("test_writeToFile", test_writeToFile),
            ("test_initWithContentsOfFile", test_initWithContentsOfFile),
-            ("test_readWriteURL", test_readWriteURL)
+            ("test_readWriteURL", test_readWriteURL),


Although in principle having a trailing comma makes for less invasive changes in the future, since you aren't actually adding anything in this line then it's not relevant for this commit. Can you revert this line change and then squash the commits?

alblue · 2017-11-02T16:22:47Z

TestFoundation/TestNSString.swift

@@ -1169,6 +1170,17 @@ class TestNSString : XCTestCase {
        let s6 = NSString(string: "Beyonce\u{301} and Tay")
        XCTAssertEqual(s6.substring(with: NSMakeRange(7, 9)), "\u{301} and Tay")
    }
+
+    func test_createCopy() {
+        //SR-6139


This comment isn't particularly relevant - as long as the SR-6139 is mentioned in the commit message, people will be able to identify that it came from this bug in the first place. Can you remove this comment?

sashabelonogov · 2017-11-02T17:35:03Z

@alblue, makes sense, sure.
The code is updated, branch is rebased and commits are squashed. Thank you for pointing it out.

parkera · 2017-11-02T19:46:23Z

Foundation/NSString.swift

+        }
+        let result = NSString()
+        result._storage = self._storage
+        return result
    }


This doesn't make a lot of sense to me. Why shouldn't an immutable NSString be allowed to return self for a copy?

@parkera you're absolutely right, I didn't think about it. I updated the commit adding the case of the immutable NSString copy, what do you think about it?

even though this method will not do what the name of it says, it makes sense to return self for performance reasons

I think the best answer here is going to depend on if we can get a factory pattern proposal through swift-evolution... the way it works in the Darwin Foundation today is that when you use any of NSString's init methods, you're most likely getting a subclass of NSString, which implements copy by returning self.

For now, I think we should do this:

if type(of: self) === NSString.self then return self

else, use one of NSString's other public initializers to create a new instance with the contents.

Don't assume result._storage exists, because a subclass would not have access to that and may not use it to hold the contents

parkera

This would have some pretty serious performance concerns.

sashabelonogov · 2017-11-06T21:53:48Z

I updated the code handling the case of the immutable NString.copy()

Points have been actioned

alblue · 2017-11-07T15:02:23Z

Seems to be OK for me now. @parkera is the proposed approach with the copy done correctly, or do we have to worry about other immutable string implementations like CF implementations?

sashabelonogov · 2017-11-07T18:22:36Z

@parkera thanks for updates, that makes total sense, I didn't think about subclasses. I updated it to this:

    open func copy(with zone: NSZone? = nil) -> Any {
        if type(of: self) === NSString.self {
            return self
        }
        if type(of: self) === NSMutableString.self {
            if let contents = _fastContents {
                return NSString(characters: contents, length: length)
            }
            let result = NSString()
            result._storage = _storage
            return result
        }
        let characters = UnsafeMutablePointer<unichar>.allocate(capacity: length)
        getCharacters(characters, range: NSMakeRange(0, length))
        let result = NSString(characters: characters, length: length)
        characters.deinitialize()
        characters.deallocate(capacity: length)
        return result
    }

What do you think? Or should I rather remove the _storage usage at all? I can also replace it with NSString(string: self._bridgeToSwift()) that will do pretty much the same.

parkera · 2017-11-07T18:54:08Z

The part about NSMutableString should be in NSMutableString itself (subclasses need to override this to provide behavior that works for them, until we have factory patterns).

The rest looks fine, I think.

sashabelonogov · 2017-11-07T19:15:52Z

The NSMutableString part is deleted. I can't disagree with "subclasses need to override this to provide behavior that works for them". It's especially interesting to notice that most of the similar methods for other classes are implemented for both cases: mutable and immutable. Thanks for the feedback!

alblue · 2017-11-10T11:47:57Z

@swift-ci please test

sashabelonogov · 2017-11-28T12:25:25Z

@parkera I'm sorry for disturbing you but what is the status with this PR? I updated it with all the changes requested by you.

parkera · 2017-11-28T23:37:36Z

Thanks for the ping. We should merge this.

alblue previously requested changes Nov 2, 2017

View reviewed changes

parkera reviewed Nov 2, 2017

View reviewed changes

parkera self-requested a review November 2, 2017 19:48

parkera requested changes Nov 2, 2017

View reviewed changes

[SR-6139] Implement NSString.copy() method

6f371a1

parkera approved these changes Nov 28, 2017

View reviewed changes

parkera merged commit 9326a69 into swiftlang:master Nov 28, 2017

swift-ci mentioned this pull request Nov 1, 2017

[SR-6139] NSArray(array:copyItems:) doesn't make a copy if requested #3799

Closed

[SR-6139] Implement NSString.copy() method #1291

[SR-6139] Implement NSString.copy() method #1291

Uh oh!

Conversation

sashabelonogov commented Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spevans commented Nov 1, 2017

Uh oh!

sashabelonogov commented Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ianpartridge commented Nov 2, 2017

Uh oh!

ianpartridge commented Nov 2, 2017

Uh oh!

spevans commented Nov 2, 2017

Uh oh!

sashabelonogov commented Nov 2, 2017

Uh oh!

spevans commented Nov 2, 2017

Uh oh!

spevans commented Nov 2, 2017

Uh oh!

alblue left a comment

Choose a reason for hiding this comment

Uh oh!

alblue Nov 2, 2017

Choose a reason for hiding this comment

Uh oh!

alblue Nov 2, 2017

Choose a reason for hiding this comment

Uh oh!

alblue Nov 2, 2017

Choose a reason for hiding this comment

Uh oh!

sashabelonogov commented Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parkera Nov 2, 2017

Choose a reason for hiding this comment

Uh oh!

sashabelonogov Nov 2, 2017

Choose a reason for hiding this comment

Uh oh!

sashabelonogov Nov 2, 2017

Choose a reason for hiding this comment

Uh oh!

parkera Nov 7, 2017

Choose a reason for hiding this comment

Uh oh!

parkera left a comment

Choose a reason for hiding this comment

Uh oh!

sashabelonogov commented Nov 6, 2017

Uh oh!

alblue commented Nov 7, 2017

Uh oh!

sashabelonogov commented Nov 7, 2017

Uh oh!

parkera commented Nov 7, 2017

Uh oh!

sashabelonogov commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alblue commented Nov 10, 2017

Uh oh!

sashabelonogov commented Nov 28, 2017

Uh oh!

parkera commented Nov 28, 2017

Uh oh!

Uh oh!

sashabelonogov commented Nov 1, 2017 •

edited

Loading

sashabelonogov commented Nov 1, 2017 •

edited

Loading

sashabelonogov commented Nov 2, 2017 •

edited

Loading

sashabelonogov commented Nov 7, 2017 •

edited

Loading