Generalize factory patterns for use outside of NSValue #1095
Conversation
|
cc @norio-nomura, after looking at this a bit further I think we can do some really neat things. Other than the bare init case did you find any other caveats worth mentioning? |
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
| // See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
| // | ||
|
|
||
| #if DEPLOYMENT_RUNTIME_SWIFT |
There was a problem hiding this comment.
This is now shareable to the Darwin overlay
|
Wow! ❤️ |
|
oh I have more incoming. Prior to your approach we had two major differentials between the general behavior of the objc version of Foundation and swift-corelibs-foundation; factory patterns and bridging. This method effectively catches 90% of the factory pattern side of things. I have a version for NSString and NSMutableString an I am currently working on getting a few more types. Overall this approach will vastly improve performance for swift-corelibs-foundation and dramatically get closer to the expected behavior for abstract base classes. |
phausler
force-pushed
the
factory_generalization
branch
from
7b14553
to
61a85c5
Compare
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
@swift-ci please test |
4 similar comments
|
@swift-ci please test |
|
@swift-ci please test |
|
@swift-ci please test |
|
@swift-ci please test |
|
@phausler can you rebase this onto the latest? In particular, the project.pbxproj has recently had conflicts (which @ianpartridge resolved elsewhere) so would be good to make sure this doesn't conflict. |
|
There are a number of other changes I have pending for this - namely it does not yet work fully on Linux, once I get back to the office next week I will revise those changes onto master and attempt to get this landed |
| @@ -187,7 +187,7 @@ CF_PRIVATE CFIndex __CFActiveProcessorCount(); | |||
| #if defined(DEBUG) | |||
| #define CFAssert(cond, prio, desc) do { if (!(cond)) { CFLog(prio, CFSTR(desc)); /* HALT; */ } } while (0) | |||
| #define CFAssert1(cond, prio, desc, a1) do { if (!(cond)) { CFLog(prio, CFSTR(desc), a1); /* HALT; */ } } while (0) | |||
| #define CFAssert2(cond, prio, desc, a1, a2) do { if (!(cond)) { CFLog(prio, CFSTR(desc), a1, a2); /* HALT; */ } } while (0) | |||
| #define CFAssert2(cond, prio, desc, a1, a2) do { if (!(cond)) { CFLog(prio, CFSTR(desc), a1, a2); HALT; } } while (0) | |||
There was a problem hiding this comment.
Why is the HALT uncommented here?
There was a problem hiding this comment.
I was using it to catch some failures in bridge casts
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Minor nit, but there's two empty lines here where one would do.
| line.append(" = ") | ||
| line.append("\"") | ||
| line += "\"" | ||
| line += " = " |
There was a problem hiding this comment.
Given that we're appending these three constants together, is there a reason we don't just have them all in one constant \" = \" instead?
| @@ -206,6 +206,10 @@ internal func NSInvalidArgument(_ message: String, method: String = #function, f | |||
| fatalError("\(method): \(message)", file: file, line: line) | |||
| } | |||
|
|
|||
| internal func _NSMethodExceptionProem(_ obj: NSObject, _ cmd: StaticString = #function) -> String { | |||
There was a problem hiding this comment.
What does the Proem mean here?
There was a problem hiding this comment.
this is taken from the actual Foundation; https://www.merriam-webster.com/dictionary/proem it is a preliminary comment
| public static let caseInsensitive = CompareOptions(rawValue: 1) | ||
| public static let literal = CompareOptions(rawValue: 2) | ||
| public static let backwards = CompareOptions(rawValue: 4) | ||
| public static let anchored = CompareOptions(rawValue: 8) | ||
| public static let numeric = CompareOptions(rawValue: 64) | ||
| public static let diacriticInsensitive = CompareOptions(rawValue: 128) | ||
| public static let widthInsensitive = CompareOptions(rawValue: 256) | ||
| public static let forcedOrdering = CompareOptions(rawValue: 512) | ||
| public static var forcedOrdering = CompareOptions(rawValue: 512) |
There was a problem hiding this comment.
Can this really change? Why is it a global var?
There was a problem hiding this comment.
hmm probably shouldn't be
| @@ -1149,7 +1455,7 @@ extension NSString { | |||
| if !getBytes(nil, maxLength: Int.max - 1, usedLength: &numBytes, encoding: enc, options: [], range: theRange, remaining: nil) { | |||
| throw NSError(domain: NSCocoaErrorDomain, code: CocoaError.fileWriteInapplicableStringEncoding.rawValue, userInfo: [ | |||
| NSURLErrorKey: dest, | |||
| ]) | |||
| ]) | |||
There was a problem hiding this comment.
Is this indentation expected? I thought it looked better before, where it was aligned with the 'throw'
There was a problem hiding this comment.
ugh yea this is what Xcode does :/
Once I have the linux side working I will give it a beautification pass
| @@ -1159,7 +1465,7 @@ extension NSString { | |||
| if !getBytes(mutableBytes, maxLength: numBytes, usedLength: &used, encoding: enc, options: [], range: theRange, remaining: nil) { | |||
| throw NSError(domain: NSCocoaErrorDomain, code: CocoaError.fileWriteUnknown.rawValue, userInfo: [ | |||
| NSURLErrorKey: dest, | |||
| ]) | |||
| ]) | |||
There was a problem hiding this comment.
I would have thought the alignment would be better if aligned with the throw
|
|
||
| let singleChar = NSString(string: "😹") | ||
| XCTAssertEqual(singleChar.substring(with: NSMakeRange(0,1)), "�") | ||
| // let surrogatePairSuffix = NSString(string: "Hurray🎉") |
There was a problem hiding this comment.
Were these test failures failing, or no longer relevant? If they're broken, do we have a bug to fix them again?
There was a problem hiding this comment.
these tests iirc don't actually work on Darwin (objc), will have to double check
…Set from the Darwin overlay
… have specialized data backing
…ional fixes for bridging
phausler
force-pushed
the
factory_generalization
branch
from
9d1264e
to
c64924e
Compare
…k correctly on linux for this use case)
…FCharacterSet linux does not properly handle the direct as cast.
|
@swift-ci please test |
|
@phausler there was a lot of work done on this branch, but it's got out of step. Can you rebase and re-run the tests please? |
|
@phausler This pull request has gone stale, and there are a number of conflicts which prevent it moving forward at this stage. I'm going to tentatively close it, and we can re-open in the future once the conflicts have been fixed. |
No description provided.