Codable conformance to some Foundation types #1108
Conversation
Foundation/CGFloat.swift
Outdated
| // other type. | ||
| do { | ||
| if NativeType.self == Float.self { | ||
| self.native = NativeType(try container.decode(Float.self)) |
There was a problem hiding this comment.
@itaiferber Could you double check this for me please?
over at https://github.com/apple/swift/blob/master/stdlib/public/SDK/CoreGraphics/CGFloat.swift.gyb#L823 it's
if NativeType.self == Float.self {
self.native = NativeType(try container.decode(Double.self))
} else {
self.native = NativeType(try container.decode(Float.self))
}But shouldn't Double.self and Float.self be the other way around?
There was a problem hiding this comment.
That is behaving correctly; it is in the fallback case where it tries to decode in the reversed order because the primary type failed.
There was a problem hiding this comment.
Philippe is correct. This is a fallback case for encoders which differentiate between Doubles and Floats in a way that makes them incompatible. On a platform where CGFloat.NativeType == Double.self, we will encode it as a Double. If we try to read it back on a platform where CGFloat.NativeType == Float.self, however, the decode could fail since the Double value would not fit in a Float. So this is that fallback case — if our native type is a Float but we failed to read it as a Float, try to read it as a Double since the decode may have failed.
The code is correct — you'll want to have the types the same way here as in the Foundation overlay.
There was a problem hiding this comment.
@phausler fix pushed
|
@swift-ci please test |
Foundation/CGFloat.swift
Outdated
| // other type. | ||
| do { | ||
| if NativeType.self == Float.self { | ||
| self.native = NativeType(try container.decode(Float.self)) |
There was a problem hiding this comment.
Philippe is correct. This is a fallback case for encoders which differentiate between Doubles and Floats in a way that makes them incompatible. On a platform where CGFloat.NativeType == Double.self, we will encode it as a Double. If we try to read it back on a platform where CGFloat.NativeType == Float.self, however, the decode could fail since the Double value would not fit in a Float. So this is that fallback case — if our native type is a Float but we failed to read it as a Float, try to read it as a Double since the decode may have failed.
The code is correct — you'll want to have the types the same way here as in the Foundation overlay.
|
@itaiferber @phausler Oh I didn't realize that. Thanks for the explanation! |
|
@itaiferber fix pushed 👌 |
|
@bubski Thanks! |
|
@swift-ci Please test |
|
Hi @bubski this is really great. Please could you explain what the problems are with |
|
@ianpartridge Sure, I don't have the full picture but I can share what I've seen so far. Oh and as for tracking, I haven't reported them anywhere yet. That's on me. I was waiting to have a bit more context and understand them better, but should have reported them. Calendar, DateComponents, TimeZone
#define TZDIR "/usr/share/zoneinfo" /* Time zone object file directory */
I'm running macOS 10.12. I don't know where to go from here and didn't have time to look into it. -- Also, there's this #274 (review) but it's not as severe as the above. CharacterSetThis discrepancy makes it hard for me to write reasonable tests. MeasurementSorry, I don't remember :( |
|
Thanks for the comprehensive update! The |
|
@ianpartridge Thanks for the info! I appreciate it and it's very helpful. I don't want to make a PR with anything that's breaking on macOS, so for some things I might have to hold off until they underlying reasons are fixed. |
|
Thanks @bubski :) Can we merge this PR? The team in IBM are waiting to start experimenting with these new conformances :) |
|
@swift-ci Please test and merge |
This PR mirrors some changes swiftlang/swift#9715
Add conformances + unit tests for
Not included in this PR, because of some problems that need to be solved