Skip to content

Re-implement float min/max in rust #42430

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
Merged
merged 1 commit into from
Jun 16, 2017
Merged

Re-implement float min/max in rust #42430

merged 1 commit into from
Jun 16, 2017

Conversation

@nagisa
Copy link
Member

@nagisa nagisa commented Jun 4, 2017

This also adds the relevant implementations into libcore.

See #42423

@rust-highfive
Copy link
Contributor

r? @aturon

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

kennytm
kennytm reviewed Jun 4, 2017
View reviewed changes
src/libcore/num/f32.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

You mean self > other here?

Copy link
Contributor

Choose a reason for hiding this comment

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

self < other so self is that minimal one, seems reasonable.

Copy link
Member Author

Choose a reason for hiding this comment

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

The other one is/was wrong. I’ll just write down some tests.

Copy link
Member

Choose a reason for hiding this comment

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

@bombless 😂

I was commenting on 896a41a9, nagisa fixed it in the new commit b9ed1c6, GitHub somehow messed up and used the old comment referring to the latest change.

@nagisa nagisa force-pushed the core-float branch 2 times, most recently from 7544f5c to 540373d Compare June 5, 2017 12:54
@natboehm
Copy link
Contributor

natboehm commented Jun 5, 2017 

[00:02:15] tidy error: Unstable library feature 'core_float_min_max' needs to have a section within the 'library features' section of The Unstable Book
[00:02:15] some tidy checks failed

Tidy is failing because this isn't mentioned in the book.

@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 nagisa force-pushed the core-float branch 2 times, most recently from 333bf76 to ef2b445 Compare June 5, 2017 15:51
@arielb1 arielb1 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 6, 2017
@carols10cents
Copy link
Member

Aturon's on vacation this week, so let's try...

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned aturon Jun 12, 2017
alexcrichton
alexcrichton reviewed Jun 13, 2017
View reviewed changes
src/libcore/num/mod.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Since these are stable in the standard library I think it's fine to go ahead and stabilize them in libcore itself, this is just moving the implementation down a layer where we could

src/libcore/num/f32.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

FWIW the musl implementation is quite different, it's not clear to me that these are the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only difference between the implementations is handling of the signed zero. IEEE 754 allows to ignore the signed-ness of zero here and, if 0.0 is compared to -0.0, to return either one.

Even the C99 standard does not mention signed zeroes in the annex referenced by musl. It quite literally paraphrases the IEEE 754 and provides an example implementation which matches (with small differences) the one provided here.

Copy link
Member

Choose a reason for hiding this comment

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

Ok seems reasonable!

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2017
@nagisa nagisa force-pushed the core-float branch 2 times, most recently from bd0be22 to b1dadc8 Compare June 14, 2017 15:26
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Jun 16, 2017

📌 Commit ba6cf1d has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Jun 16, 2017

⌛ Testing commit ba6cf1d with merge fe7227f...

bors added a commit that referenced this pull request Jun 16, 2017
@bors
Auto merge of #42430 - nagisa:core-float, r=alexcrichton
fe7227f 
Re-implement float min/max in rust

This also adds the relevant implementations into libcore.

See #42423
@nagisa nagisa added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 16, 2017
@bors
Copy link
Collaborator

bors commented Jun 16, 2017

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

@bors bors merged commit ba6cf1d into rust-lang:master Jun 16, 2017
@bors bors mentioned this pull request Jun 16, 2017
Closed
@sunfishcode sunfishcode mentioned this pull request Oct 18, 2017
Closed
@est31 est31 mentioned this pull request Feb 10, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcrichton alexcrichton alexcrichton left review comments

@kennytm kennytm kennytm left review comments

+1 more reviewer

@bombless bombless bombless left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@alexcrichton alexcrichton

Labels

relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@nagisa @rust-highfive @natboehm @carols10cents @alexcrichton @bors @kennytm @bombless @aturon @arielb1