O(n) replacement in NSString.replaceOccurrances #1620
Conversation
|
I don't see any existing tests for this method, could you add some test cases? |
|
@spevans these tests exercise the change: |
Foundation/NSString.swift
Outdated
| for cnt in 0..<numOccurrences { | ||
| let rangePtr = CFArrayGetValueAtIndex(findResults, backwards ? cnt : numOccurrences - cnt - 1) | ||
| replaceCharacters(in: NSRange(rangePtr!.load(as: CFRange.self)), with: replacement) |
There was a problem hiding this comment.
removing this changes the behavior for subclassing
There was a problem hiding this comment.
@phausler You're right. Not using replaceCharacters will change behaviour for subclassing, but using it will make replaceOccurrences O(n*m). We'd need non-contiguous _storage to resolve that performance issue.
Can you suggest a way out?
There was a problem hiding this comment.
Currently the concrete class is NsMutableString itself so ‘type(of: self) === NSMutableString.self’ would be true for the concrete case but not the subclass case
There was a problem hiding this comment.
Checking the class seems like a reasonable workaround to me.
There was a problem hiding this comment.
Can you use something like the following but specialise the substring(with:) for NSMutableString to avoid the String() copy it uses:
var newStorage = ""
var currentIndex = 0
for cnt in 0..<numOccurrences {
let rangePtr = CFArrayGetValueAtIndex(findResults, backwards ? numOccurrences - cnt - 1 : cnt)
let range = NSRange(rangePtr!.load(as: CFRange.self))
let copyRange = NSRange(location: currentIndex, length: range.location)
newStorage += substring(with: copyRange)
newStorage += replacement
currentIndex = range.location + range.length
}
let copyRange = NSRange(location: currentIndex, length: length - currentIndex)
newStorage += substring(with: copyRange)
setString(newStorage)
Foundation/NSString.swift
Outdated
| @@ -1423,10 +1423,20 @@ extension NSMutableString { | |||
|
|
|||
| if let findResults = CFStringCreateArrayWithFindResults(kCFAllocatorSystemDefault, _cfObject, target._cfObject, CFRange(searchRange), options._cfValue(true)) { | |||
| let numOccurrences = CFArrayGetCount(findResults) | |||
| let start = _storage.utf16.startIndex | |||
There was a problem hiding this comment.
_storage should only be accessed in concrete methods, it may not always be there
There was a problem hiding this comment.
This might be tricky, without factory patterns which allow us to return a special NSMutableString subclass.
gmilos
force-pushed
the
SR-7749-NSString.replaceOccurrances-improvements
branch
from
8a75d5a to
23a8abf
Compare
|
@swift-ci please test and merge |
|
@swift-ci please test and merge |
|
Thanks for the ping. I’ll put it back on its merry way when CI unblocks. |
|
@swift-ci please test and merge |
|
@gmilos looks like this change needs to be rebased |
|
|
||
| override func character(at index: Int) -> unichar { | ||
| let start = _storage.utf16.startIndex | ||
| return _storage.utf16[start.advanced(by: index)] |
There was a problem hiding this comment.
This style of indexing is not available in 4.2 and 5.0. Can we fix this?
|
@gmilos can you rebase your change so that we can retest? |
|
ping @gmilos |
1 similar comment
|
ping @gmilos |
…n corelibs-foundation This supercedes swiftlang#1620.
…n corelibs-foundation This supercedes swiftlang#1620.
…n corelibs-foundation This supercedes swiftlang#1620.
…n corelibs-foundation This supercedes swiftlang#1620.
…n corelibs-foundation This supercedes swiftlang#1620.
…n corelibs-foundation This supercedes swiftlang#1620. (cherry picked from commit 4cd59de)
…th:) in corelibs-foundation This supercedes swiftlang#1620. (cherry picked from commit 4cd59de)
…th:) in corelibs-foundation This supercedes swiftlang#1620. (cherry picked from commit 4cd59de) (cherry picked from commit 1e71e90)
Bug discussed here:
https://bugs.swift.org/browse/SR-7749