Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix NaN handling in is_sign_negative/positive #42431

Merged
merged 1 commit into from
Jun 28, 2017
Merged

Fix NaN handling in is_sign_negative/positive #42431

merged 1 commit into from
Jun 28, 2017

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Jun 4, 2017 •
edited
Loading

This would be my proposed fix for the #42425 provided we decide it is indeed a problem.

Note this would technically be a breaking change to a stable API. We might want to consider deprecating these methods and adding new ones.

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@kennytm kennytm mentioned this pull request Jun 4, 2017
Merged
retep998
retep998 reviewed Jun 4, 2017
View reviewed changes
src/libcore/num/f32.rs Outdated
@@ -205,18 +205,25 @@ impl Float for f32 {
}
}

/// Returns `true` if `self` is positive, including `+0.0` and
/// `Float::infinity()`.
/// Returns `true` if and only if `self` is has a positive sign, including `-0.0`, `NaN`s with
Copy link
Member

Choose a reason for hiding this comment

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

is has?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm more surprised that it returns true for "-0.0, NaNs with negative sign, and negative infinity". I would not expect those cases to have a positive sign.

@nagisa nagisa force-pushed the core-float-2 branch 2 times, most recently from 8c1ee68 to 0853ec5 Compare June 4, 2017 23:29
@natboehm
Copy link
Contributor

natboehm commented Jun 5, 2017 

[01:11:11] failures:
[01:11:11]     f32::tests::test_is_sign_positive
[01:11:11]     f32::tests::test_nan
[01:11:11]     f64::tests::test_is_sign_positive
[01:11:11]     f64::tests::test_nan

It looks like some tests are failing.

@carols10cents carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 5, 2017
@nagisa
Copy link
Member Author

nagisa commented Jun 5, 2017 •
edited
Loading

This is not supposed to be in a landable state until we reach some concensus anyway.

@arielb1 arielb1 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 6, 2017
@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 6, 2017
@arielb1
Copy link
Contributor

arielb1 commented Jun 20, 2017

What's the status of this @nagisa? Just checking in to make sure it isn't getting lost.

@alexcrichton
Copy link
Member

Discussed during libs triage the conclusion was that this seems good to land! @nagisa want to fix up the tests and I'll r+?

@alexcrichton alexcrichton added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jun 22, 2017
@alexcrichton
Copy link
Member

Looks like there's some Travis errors?

[01:11:14] failures:
[01:11:14] 
[01:11:14] ---- f32.rs - f32::f32::is_sign_negative (line 388) stdout ----
[01:11:14] 	thread 'rustc' panicked at 'test executable failed:
[01:11:14] 
[01:11:14] thread 'main' panicked at 'assertion failed: !nan.is_sign_positive() && !nan.is_sign_negative()', <anon>:13
[01:11:14] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:11:14] 
[01:11:14] ', /checkout/src/librustdoc/test.rs:318
[01:11:14] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:11:14] 
[01:11:14] ---- f32.rs - f32::f32::is_sign_positive (line 369) stdout ----
[01:11:14] 	thread 'rustc' panicked at 'test executable failed:
[01:11:14] 
[01:11:14] thread 'main' panicked at 'assertion failed: !nan.is_sign_positive() && !nan.is_sign_negative()', <anon>:13
[01:11:14] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:11:14] 
[01:11:14] ', /checkout/src/librustdoc/test.rs:318
[01:11:14] 
[01:11:14] ---- f64.rs - f64::f64::is_sign_negative (line 332) stdout ----
[01:11:14] 	thread 'rustc' panicked at 'test executable failed:
[01:11:14] 
[01:11:14] thread 'main' panicked at 'assertion failed: !nan.is_sign_positive() && !nan.is_sign_negative()', <anon>:14
[01:11:14] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:11:14] 
[01:11:14] ', /checkout/src/librustdoc/test.rs:318
[01:11:14] 
[01:11:14] ---- f64.rs - f64::f64::is_sign_positive (line 307) stdout ----
[01:11:14] 	thread 'rustc' panicked at 'test executable failed:
[01:11:14] 
[01:11:14] thread 'main' panicked at 'assertion failed: !nan.is_sign_positive() && !nan.is_sign_negative()', <anon>:14
[01:11:14] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:11:14] 
[01:11:14] ', /checkout/src/librustdoc/test.rs:318
[01:11:14] 
[01:11:14] 
[01:11:14] failures:
[01:11:14]     f32.rs - f32::f32::is_sign_negative (line 388)
[01:11:14]     f32.rs - f32::f32::is_sign_positive (line 369)
[01:11:14]     f64.rs - f64::f64::is_sign_negative (line 332)
[01:11:14]     f64.rs - f64::f64::is_sign_positive (line 307)
[01:11:14] 
[01:11:14] test result: FAILED. 820 passed; 4 failed; 22 ignored; 0 measured; 0 filtered out

@arielb1
Copy link
Contributor

arielb1 commented Jun 27, 2017

@nagisa is this ready for review?

@nagisa nagisa added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 27, 2017
@nagisa
Copy link
Member Author

nagisa commented Jun 27, 2017

@alexcrichton tests pass.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 27, 2017

📌 Commit 3b9fe77 has been approved by alexcrichton

@alexcrichton alexcrichton reopened this Jun 27, 2017
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 27, 2017

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Jun 27, 2017

📌 Commit 3b9fe77 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jun 28, 2017

⌛ Testing commit 3b9fe77 with merge 88c3242...

bors added a commit that referenced this pull request Jun 28, 2017
@bors
Auto merge of #42431 - nagisa:core-float-2, r=alexcrichton
88c3242 
Fix NaN handling in is_sign_negative/positive

This would be my proposed fix for the #42425 provided we decide it is indeed a problem.

Note this would technically be a breaking change to a stable API. We might want to consider deprecating these methods and adding new ones.
@bors
Copy link
Contributor

bors commented Jun 28, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 88c3242 to master...

@bors bors merged commit 3b9fe77 into rust-lang:master Jun 28, 2017
@cuviper cuviper mentioned this pull request Jun 29, 2017
Closed
@PlasmaPower PlasmaPower mentioned this pull request Jul 8, 2017
Merged
@kennytm kennytm mentioned this pull request Aug 3, 2017
Merged
@arielb1 arielb1 mentioned this pull request Aug 5, 2017
Closed
@hanna-kruppe hanna-kruppe mentioned this pull request Nov 20, 2017
Closed
@tspiteri tspiteri mentioned this pull request Dec 29, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@retep998 retep998 retep998 left review comments

@Thiez Thiez Thiez left review comments

Assignees

@brson brson

Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@nagisa @rust-highfive @natboehm @arielb1 @alexcrichton @bors @Thiez @retep998 @brson @carols10cents