Fix for [SR-1250] NSJSONSerialization emits non-floating-point number… #914
Conversation
|
@swift-ci please test |
|
Wouldn't this change impact existing code, which would assume that such numbers are doubles? Does this behaviour also exist on Darwin platforms? |
|
@alblue : Yeah ..That's what I observed on Darwin ..I updated the description with details.In my fix am just converting to |
|
This is all tied up in the same discussion about what the proper role of NSNumber is in Swift (cc @phausler), but in the meantime it looks reasonable to me. |
saiHemak
force-pushed
the
nsjson-serialization
branch
from
0423af9 to
8a1bd9a
Compare
Foundation/NSJSONSerialization.swift
Outdated
| } | ||
| guard doubleDistance > 0 else { | ||
| return nil | ||
| if doubleResult == roundedDouble || doubleResult == 0.0 { |
There was a problem hiding this comment.
Why is the second half of this if statement necessary? Shouldn't this be if doubleResult == doubleResult.rounded()?
Foundation/NSJSONSerialization.swift
Outdated
| guard intDistance > 0 || doubleDistance > 0 else { | ||
| return nil | ||
| } | ||
|
|
||
| if intDistance == doubleDistance { |
There was a problem hiding this comment.
Removing these lines seems suspect. First, intResult is now unused. Second--more importantly--there are values of Int that cannot be exactly represented in Double (those above 2^53), which is why intResult isn't totally redundant given Int(doubleResult). Was there a reason why you removed these lines?
You will almost certainly want the second guard statement below before proceeding to use doubleResult.
There was a problem hiding this comment.
@xwu Thanks for the review. I agree with you .I have missed to test with huge integers and thought that it would be redundant .We need to retain the intResult too and added the check to handle of rounded doubles . I have modified the fix now ..
| @@ -468,7 +468,7 @@ extension TestNSJSONSerialization { | |||
| XCTAssertEqual(result?[1] as? Int, -1) | |||
| XCTAssertEqual(result?[2] as? Double, 1.3) | |||
| XCTAssertEqual(result?[3] as? Double, -1.3) | |||
| XCTAssertEqual(result?[4] as? Double, 1000) | |||
| XCTAssertEqual(result?[4] as? Int, 1000) | |||
There was a problem hiding this comment.
Nit: please follow file convention and align 1000 with the line above.
| do { | ||
| let data = decimalArray.data(using: String.Encoding.utf8) | ||
| let result = try JSONSerialization.jsonObject(with: data!, options: []) as? [Any] | ||
| XCTAssertEqual(result?[0] as! Double,12.1) |
There was a problem hiding this comment.
Nit: can you put a space after the comma here and on subsequent lines?
| } | ||
| } | ||
|
|
||
| fileprivate func createTestFile(_ path: String,_contents: Data) -> String? { |
There was a problem hiding this comment.
Please restore the original indent of four spaces and not three here.
Foundation/NSJSONSerialization.swift
Outdated
| let intResult = strtol(startPointer, intEndPointer, 10) | ||
| let intDistance = startPointer.distance(to: intEndPointer[0]!) | ||
| let doubleResult = strtod(startPointer, doubleEndPointer) | ||
| let doubleDistance = startPointer.distance(to: doubleEndPointer[0]!) | ||
|
|
||
| let roundedDouble = doubleResult.rounded() |
There was a problem hiding this comment.
At minimum, this is best moved after the guard doubleDistance > 0 statement that should be restored below. As mentioned earlier, you may not need to have this line at all.
saiHemak
force-pushed
the
nsjson-serialization
branch
from
8a1bd9a to
3f6938f
Compare
|
@swift-ci please test |
saiHemak
force-pushed
the
nsjson-serialization
branch
from
3f6938f to
eb8fa91
Compare
saiHemak
force-pushed
the
nsjson-serialization
branch
from
eb8fa91 to
b8f0af1
Compare
|
@phausler Please review |
|
This seems reasonable, but for compatability it is worth noting that NSNumbers returned are a bit strange in Swift. let n = NSNumber(value: 4.3)
if n is Int {
// this is true as of the current build of Swift on Darwin... which imho seems a bit incorrect.
} |
JSON serialization behaves differently on Linux.
eg : Serializing
let subject = "[12.1,10.0,0.0]"on Darwin gives - 12.1,10,0
where as on Linux - 12.1,10.0,0.0 //.0 needs to be truncated
This fix aligns the
JSONSerialization.jsonObject(with: data)behavior to Darwin.