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.

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

Tests for f32/f64 float methods #22984

Merged
merged 3 commits into from
Mar 8, 2015
Merged

Tests for f32/f64 float methods #22984

merged 3 commits into from
Mar 8, 2015

Conversation

carols10cents
Copy link
Member

Building on #22076, I've added some tests for stable methods in f32 and f64 that didn't have any before.

Please let me know if there are any improvements I can make, and I am happy to make them! 📬

@rust-highfive
Copy link
Collaborator

r? @aturon

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

@huonw
Copy link
Member

huonw commented Mar 3, 2015

Wow!

@carols10cents
Copy link
Member Author

Is that a good wow or a bad wow? :)

@huonw
Copy link
Member

huonw commented Mar 3, 2015

Definitely a good one! The patch is very comprehensive.

@carols10cents
Copy link
Member Author

Yay!!!! Thank you :)

assert_eq!(0.0, zero);
assert!(!zero.is_infinite());
assert!(zero.is_finite());
assert!(zero.is_positive());
Copy link
Member

Choose a reason for hiding this comment

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

Hm... not a problem with this patch at all, since this behaviour is documented, but I'm unsure if considering zero to be positive is a great idea. (I filed #22985.)

Maybe you ensure that all the f32/f64 tests of this nature test both is_positive and is_negative, though. I.e. check they're false when they should be, as well as checking that they're true when they should be. (Including the NaN/one ones.)

@huonw
Copy link
Member

huonw commented Mar 3, 2015

A few minor comments (I've only put them on f32 but they apply to f64 as well) and it should be good to go.

@huonw huonw assigned huonw and unassigned aturon Mar 3, 2015
@carols10cents
Copy link
Member Author

@huonw Thank you so much for the thorough review! 😻

I added new commits to address your comments; please let me know if you'd like me to squash them down into 902309a :)

@huonw
Copy link
Member

huonw commented Mar 4, 2015

Looks good; squashing would be great.

@carols10cents
Copy link
Member Author

Done!! :)

@bors
Copy link
Contributor

bors commented Mar 6, 2015

☔ The latest upstream changes (presumably #23031) made this pull request unmergeable. Please resolve the merge conflicts.

I was having trouble figuring out which functions had tests and which
didn't. This commit is just moving tests around and does not change
anything.
@carols10cents
Copy link
Member Author

@bors thanks for letting me know! ❤️ good bot.

@steveklabnik
Copy link
Member

@bors: r=huonw 1bda1ff

@bors
Copy link
Contributor

bors commented Mar 8, 2015

⌛ Testing commit 1bda1ff with merge b775541...

bors added a commit that referenced this pull request Mar 8, 2015
Building on #22076, I've added some tests for stable methods in f32 and f64 that didn't have any before.

Please let me know if there are any improvements I can make, and I am happy to make them! 📬
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 8, 2015
 Building on rust-lang#22076, I've added some tests for stable methods in f32 and f64 that didn't have any before.

Please let me know if there are any improvements I can make, and I am happy to make them! 📬
@bors bors merged commit 1bda1ff into rust-lang:master Mar 8, 2015
@carols10cents carols10cents deleted the tests-for-float branch March 9, 2015 01:20
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.

6 participants