NSNumber already preserves whether a value was originally boolean. #4366
Conversation
|
@swift-ci Please test OS X platform |
|
I wish @gribozavr were around to review this, but he's not. @DougGregor, what do you think about this instead of #4363? |
|
and @phausler probably has good thoughts here as well. |
| result = ((_SwiftTypePreservingNSNumber *) self_)->tag; | ||
| } else if (CFGetTypeID(self_) == CFBooleanGetTypeID()) { |
There was a problem hiding this comment.
this probably should be a strcmp of self.objCType and @encode(BOOL)
There was a problem hiding this comment.
assert(strcmp(@YES.objCType, @encode(BOOL)) == 0);
where as CFGetTypeID of a subclass of NSNumber may not return CFBooleanGetTypeID() unless _cfTypeID is overridden.
|
a side-concern; doesn't _SwiftTypePreservingNSNumber destroy our optimizations for tagged pointers from Foundation? |
|
It does for now. I figure if/when Swift and Foundation are revlocked we may be able to hardcode those as well. The whole point of this type is to tell whether a particular value was originally a boolean. It's an awful compromise for when you have a Swift dictionary whose key is a particular number type, and then you bridge it to Objective-C and then back into a dictionary whose key is AnyHashable. The value types and NSNumber don't compare the same (is |
|
So basically any subclasser in this case will not get proper behavior unless they override a private method then. (Granted the number of subclassers is likely not very often) One other consideration (modulo subclasses like _SwiftTypePreservingNSNumber) NSNumber returns singletons even on 32 bit for boolean values. Overall I think NSNumber expresses something quite different than the numeric primitives and we should consider removing the bridge for it. Per the comparison; is that via assert([[NSNumber numberWithDouble:42.0] compare:@42] == NSOrderedSame); // passes |
|
Right, we don't want At this point I think it's too late to remove NSNumber bridging for Swift 3, and the reason we've always shied away from that is the very thing that prompted this change: plists. (Which also don't follow |
|
I had left that out because the new logic isn't ours; it's CF's. I guess calling CFGetTypeID doesn't guarantee that, though. |
Use that instead of rolling it up in _SwiftTypePreservingNSNumber so that we get the right behavior when we go to write plists. https://bugs.swift.org/browse/SR-2381
|
@swift-ci Please test OS X platform |
|
I don't think we'd want to ever remove |
|
@swift-ci Please test OSX platform |
|
@swift-ci Please test macOS platform |
|
... it's not triggering ... @swift-ci Please test macOS |
|
:-( It was working, just not updating GitHub. Mishal's been investigating that. The actual test is still going; I'll kill your other ones. |
…wiftlang#4366) Use that instead of rolling it up in _SwiftTypePreservingNSNumber so that we get the right behavior when we go to write plists. https://bugs.swift.org/browse/SR-2381 (cherry picked from commit ba3de9e)
Use that instead of rolling it up in
_SwiftTypePreservingNSNumberso that we get the right behavior when we go to write plists.https://bugs.swift.org/browse/SR-2381
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
A smoke test on macOS does the following:
device standard libraries are not built.
version of these tests are not run.
A smoke test on Linux does the following:
tests are not run.
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.