Skip to content

SR-8407: NSNumber as? Bool should only work for 0 or 1 #1642

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 31, 2018

Conversation

spevans
Copy link
Contributor

@spevans spevans commented Jul 31, 2018

  • NSNumber as? Bool should only work for NSNumber values 0 and 1 to
    match Darwin.

- NSNumber as? Bool should only work for NSNumber values 0 and 1 to
  match Darwin.
@spevans
Copy link
Contributor Author

spevans commented Jul 31, 2018

@swift-ci please test

@spevans spevans requested a review from millenomi July 31, 2018 11:37
@parkera parkera requested a review from phausler July 31, 2018 16:31
@parkera
Copy link
Contributor

parkera commented Jul 31, 2018

I recall @phausler doing a lot of work in this area last year as far as bridging goes; do we need to update it elsewhere here now that we have more consistent bridging with linux?

Copy link
Contributor

@phausler phausler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should follow up with a correction of the Darwin compat for 32 bit truncation on boolValue.


XCTAssertEqual(NSNumber(value: Int64.max).boolValue, true)
XCTAssertEqual(NSNumber(value: Int64.max - 1).boolValue, true)
XCTAssertEqual(NSNumber(value: Int64.min).boolValue, false) // Darwin compatibility
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is no longer the case, we finally fixed that in High Sierra.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? I tested on HS 10.13.6:

print(NSNumber(value: Int64.min).boolValue)
false


XCTAssertEqual(NSNumber(value: Int.max).boolValue, true)
XCTAssertEqual(NSNumber(value: Int.max - 1).boolValue, true)
XCTAssertEqual(NSNumber(value: Int.min).boolValue, false) // Darwin compatibility
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto here

@@ -623,6 +624,37 @@ class TestNSNumberBridging : XCTestCase {
XCTAssertEqual(value, ns_value)
}
}

func testNSNumberToBool() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is correct behavior, thanks for the update!

@spevans
Copy link
Contributor Author

spevans commented Jul 31, 2018

@parkera The behaviour for as? Bool now matches Darwin, although I think when the bridging was merged to master and swift-4.2-branch it actually regressed the previous behaviour which as also correct, so this probably warrants a cherry-pick to 4.2 as well.

@parkera
Copy link
Contributor

parkera commented Jul 31, 2018

Ok, we can do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants